Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: generate MSVC project files via python script #14062

Merged
merged 1 commit into from Aug 26, 2018

Conversation

Projects
None yet
9 participants
@ken2812221
Copy link
Member

commented Aug 25, 2018

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.

@ken2812221 ken2812221 force-pushed the ken2812221:msvc-autogen branch Aug 25, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #14007 (appveyor: Run functional test on appveyor by ken2812221)
  • #13945 (Refactoring CRPCCommand with enum category by isghe)
  • #13942 (refactor: Removal of circular dependency between index/txindex, validation and index/base by mgrychow)

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 referenced this pull request Aug 25, 2018

Closed

Utxoscriptindex #14035

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 25, 2018

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?

build_msvc/msvc-autogen.py Outdated
import os
import re

SOURCE_DIR=os.path.abspath(os.path.join(os.path.dirname(__file__), '..', 'src'))

This comment has been minimized.

Copy link
@practicalswift

practicalswift Aug 25, 2018

Member

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

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Aug 26, 2018

Author Member

Fixed

@ken2812221 ken2812221 force-pushed the ken2812221:msvc-autogen branch to 0b16f67 Aug 26, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2018

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

utACK 0b16f67

@MarcoFalke MarcoFalke merged commit 0b16f67 into bitcoin:master Aug 26, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

MarcoFalke added a commit that referenced this pull request Aug 26, 2018

Merge #14062: build: generate MSVC project files via python script
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 ken2812221:msvc-autogen branch Aug 26, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

thanks for doing this, post-mortem ACK

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

@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

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

@NicolasDorier You need python anyway for the tests

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

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

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2018

@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 = [

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Aug 29, 2018

Contributor

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.

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Sep 1, 2018

Author Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.