Skip to content

build: generate MSVC project files via python script#14062

Merged
maflcko merged 1 commit into
bitcoin:masterfrom
ken2812221:msvc-autogen
Aug 26, 2018
Merged

build: generate MSVC project files via python script#14062
maflcko merged 1 commit into
bitcoin:masterfrom
ken2812221:msvc-autogen

Conversation

@ken2812221

@ken2812221 ken2812221 commented Aug 25, 2018

Copy link
Copy Markdown
Contributor

The reason that I move from original *.vcxproj to template *.vcxproj.in file:

  • There are many developers does not know how to edit .vcxproj file
  • To keep consistency, don't need to edit file at two different places

Now the devs do not have to update two seperate files.

@DrahtBot

DrahtBot commented Aug 25, 2018

Copy link
Copy Markdown
Contributor
Note to reviewers: This pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot mentioned this pull request Aug 25, 2018
@practicalswift

Copy link
Copy Markdown
Contributor

Concept ACK

This is excellent! I think auto-generation is the only practical way to solve this for the reasons you listed.

Can we test it against recent PR:s that broke appveyor due to files being added/removed to verify that this would have solved those cases?

Comment thread build_msvc/msvc-autogen.py Outdated

@practicalswift practicalswift Aug 25, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Missing whitespace around =.

The flake8 rule set enforced by Travis is a bit lax, but since this is brand new code I suggest following all the advice given by flake8 to avoid any Python style nits :-)

$ flake8 --ignore=E501 msvc-autogen.py # E501: line length
msvc-autogen.py:6:11: E225 missing whitespace around operator
msvc-autogen.py:27:1: E302 expected 2 blank lines, found 1
msvc-autogen.py:29:20: E225 missing whitespace around operator
msvc-autogen.py:33:79: E713 test for membership should be 'not in'
msvc-autogen.py:36:32: E225 missing whitespace around operator
msvc-autogen.py:39:39: E231 missing whitespace after ','
msvc-autogen.py:41:32: E225 missing whitespace around operator
msvc-autogen.py:45:1: E302 expected 2 blank lines, found 1
msvc-autogen.py:58:1: E305 expected 2 blank lines after class or function definition, found 1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@practicalswift

Copy link
Copy Markdown
Contributor

utACK 0b16f67

A minor style nit: Personally I prefer string construction via “new style” str.format (introduced in Python 3 and backported to Python 2.7) instead of raw string concaternation (”hello “ + s + “!”).

@ken2812221

Copy link
Copy Markdown
Contributor Author

Now the master branch has failed while this PR is success.

@maflcko

maflcko commented Aug 26, 2018

Copy link
Copy Markdown
Member

utACK 0b16f67

@maflcko
maflcko merged commit 0b16f67 into bitcoin:master Aug 26, 2018
maflcko pushed a commit that referenced this pull request Aug 26, 2018
0b16f67 auto generate MSVC project files (Chun Kuan Lee)

Pull request description:

  The reason that I move from original `*.vcxproj` to template `*.vcxproj.in` file:
  - There are many developers does not know how to edit .vcxproj file
  - To keep consistency, don't need to edit file at two different places

  Now the devs do not have to update two seperate files.

Tree-SHA512: ab06dbec588cab57f16c1993ea80ed25a49b0b129884634512a8bcd8a21a1a55d38636922489bcf9120d504cfc2cbe4d2b888a217c4e65a50555b41fcd3b7004
@ken2812221
ken2812221 deleted the msvc-autogen branch August 26, 2018 16:47
@laanwj

laanwj commented Aug 27, 2018

Copy link
Copy Markdown
Member

thanks for doing this, post-mortem ACK

@NicolasDorier

NicolasDorier commented Aug 27, 2018

Copy link
Copy Markdown
Contributor

@ken2812221 would you mind if I make a PR replacing msvc-autogen.py with a powershell script? It would be nice to not need python dependency for windows developers. Thanks a lot for taking the time to work on vs support.

@sipa

sipa commented Aug 27, 2018

Copy link
Copy Markdown
Member

@NicolasDorier You need python anyway for the tests

@NicolasDorier

Copy link
Copy Markdown
Contributor

Indeed. I was under the impression the python tests were still linux only.

@ken2812221

Copy link
Copy Markdown
Contributor Author

@NicolasDorier I am just working on #14007, the functional tests is available on Windows in the PR.

would you mind if I make a PR replacing msvc-autogen.py with a powershell script?

I'll try, but I am not familiar with powershell.

'libbitcoin_zmq',
]

ignore_list = [

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the reason for the ignore list is to prevent conflicts between .obj file names. But ignoring these files is not sufficient if the code in these files actually needs to be linked. Not sure if this issue affects anybody else, but I have a fix for it in 06d939c from #10973.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's a pretty good improvement. would allow 2 cpp files with same filename.

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants