build: generate MSVC project files via python script #14062

pull ken2812221 wants to merge 1 commits into bitcoin:master from ken2812221:msvc-autogen changing 11 files +80 −131

ken2812221 commented at 6:35 PM on August 25, 2018: 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.
ken2812221 force-pushed on Aug 25, 2018
DrahtBot commented at 9:17 PM on August 25, 2018: member
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.
practicalswift commented at 9:29 PM on August 25, 2018: 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?

in build_msvc/msvc-autogen.py:6 in 18b2ea77ec outdated

   0 | @@ -0,0 +1,59 @@
   1 | +#!/usr/bin/env python3
   2 | +
   3 | +import os
   4 | +import re
   5 | +
   6 | +SOURCE_DIR=os.path.abspath(os.path.join(os.path.dirname(__file__), '..', 'src'))

practicalswift commented at 10:09 PM on August 25, 2018:

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

ken2812221 commented at 3:55 PM on August 26, 2018:

Fixed

fanquake added the label Windows on Aug 25, 2018
fanquake added the label Build system on Aug 25, 2018
auto generate MSVC project files 0b16f679d5
ken2812221 force-pushed on Aug 26, 2018
practicalswift commented at 4:15 PM on August 26, 2018: contributor

utACK 0b16f679d519370be9d2c10fb7e8f169770e5d29

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 commented at 4:17 PM on August 26, 2018: contributor

Now the master branch has failed while this PR is success.
MarcoFalke commented at 4:38 PM on August 26, 2018: member

utACK 0b16f67
MarcoFalke merged this on Aug 26, 2018
MarcoFalke closed this on Aug 26, 2018
MarcoFalke referenced this in commit 427253cf7e on Aug 26, 2018
ken2812221 deleted the branch on Aug 26, 2018
laanwj commented at 3:47 AM on August 27, 2018: member

thanks for doing this, post-mortem ACK
NicolasDorier commented at 4:48 AM on August 27, 2018: 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 commented at 4:50 AM on August 27, 2018: member

@NicolasDorier You need python anyway for the tests
NicolasDorier commented at 5:03 AM on August 27, 2018: contributor

Indeed. I was under the impression the python tests were still linux only.
ken2812221 commented at 6:09 AM on August 27, 2018: contributor

@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.
in build_msvc/msvc-autogen.py:18 in 0b16f679d5
```
  13 | +    'libbitcoin_util',
  14 | +    'libbitcoin_wallet',
  15 | +    'libbitcoin_zmq',
  16 | +]
  17 | +
  18 | +ignore_list = [
```
ryanofsky commented at 7:04 PM on August 29, 2018:

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 06d939c42bd6abf5211c159c6e5708a33ffe0077 from #10973.

ken2812221 commented at 1:06 PM on September 1, 2018:

Thanks, that's a pretty good improvement. would allow 2 cpp files with same filename.
linuxsh2 referenced this in commit dc19139c12 on Jul 29, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels