fix setup.py on Windows#294
Conversation
|
Hello @flothesof! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2019-11-27 15:20:33 UTC |
Codecov Report
@@ Coverage Diff @@
## devel #294 +/- ##
==========================================
+ Coverage 41.05% 41.11% +0.05%
==========================================
Files 79 79
Lines 23468 23432 -36
==========================================
- Hits 9634 9633 -1
+ Misses 13834 13799 -35
Continue to review full report at Codecov.
|
Didou09
left a comment
Didou09
left a comment
There was a problem hiding this comment.
I'm not sure putting updateversion into setup.py is the best move, it prevents us from running it without import setup.py.
We first need to understand better why the source installation on Windows suddenly stopped working
| def get_version_tofu(path=_HERE): | ||
|
|
||
|
|
||
| def updateversion(path=_HERE): |
There was a problem hiding this comment.
Yes and no...
I agree having updateversion defined in a standalone side module is a bit cumbersome because it seems it is only needed in the setup.py.
But this was done in order to allow travis (or us) run updateversion as a standalone without having to import setup.py (which would run it).
updateversion can be useful as a standalone when we want to force an update of the version number prior to running the setup.py, using for example:
python -c "import _updateversion as up; out = up.updateversion(); print(out)"
This is not possible anymore with this change.
Are you sure there is no other fix for issue #292 ? It used to work before, I don't get why it stopped working
There was a problem hiding this comment.
Okay, thanks for reviewing. So it seems this should not be merged as such.
I believe then this needs more investigation.
I have no idea why it's broken under Windows. Too bad...
There was a problem hiding this comment.
Nice,
Please also make sure the PR does not include the chages from PR #292 (i.e.: updateversion)
These are two independent changes, and as we discussed, I'm sure sure we can merge PR #292 as of now.
I suspect it is because you created your branch from the branch you had already created for PR #292 , right ?
Best practice : always branch from devel, a world where things are stable and validated, to make small incremental changes ;-)
|
So I'm confused, is this solution working now ? |
|
No, this is broken and will not be fixed with this PR. I think @Didou09 was talking about PR #296 which incorporated these changes. I rewrote my history on #296 to take them out. Side note: I checked out from my previous branch to get an updated .gitignore since it was a common change :) Closing since this will fix the problem. |
5 participants
This fixes the install problem I found in #292 when installing from source on Windows.
As a side note, it seems the setup.py is starting to get bloated. In particular, I noticed that
get_version_tofuandupdateversiondo something very similar (like checking if git is working in the path to know if we are on master...). This would be a an area for refactoring.