Improvements to the configuration logic - #744
Conversation
Make this in line with output from the start scripts.
As there is a subtle difference when running ./configure directly (dev config is being used) and ./configure being run by the scancode script (dev config not being used), print the config being usd in any case. This is usuall is just two lines and does not really clutter the output.
This way the explanation is also visible without looking at the source code.
pombredanne
left a comment
There was a problem hiding this comment.
Thanks! Do you think this is worth having for Windows too? (IMHO not) ... And what could be the automated test we could setup for this?
|
|
||
| SCANCODE_CONFIG_DIR="etc/conf" | ||
|
|
||
| GIT_STATUS=$(git -C "$SCANCODE_ROOT_DIR" status --porcelain 2> /dev/null) |
There was a problem hiding this comment.
Does this also implicitly checks for Git being available locally? Just curious what the different with a plain git status?
There was a problem hiding this comment.
Exactly. It implicitly checks for $SCANCODE_ROOT_DIR to be a Git working tree (git status returns code 128 otherwise), and also for the git executable to be present (in this case it's Bash that returns code 127).
| GIT_COMMIT_HEAD=$(git -C "$SCANCODE_ROOT_DIR" rev-parse HEAD) | ||
| if [ "$GIT_COMMIT_CONFIGURED" != "$GIT_COMMIT_HEAD" ]; then | ||
| echo "* (Re-)Configuring ScanCode because Git commits have changed..." | ||
| CONFIGURE_QUIET=1 $SCANCODE_ROOT_DIR/configure $SCANCODE_CONFIG_DIR |
There was a problem hiding this comment.
IMHO to be really safe, you should first issue a configure --clean but let see first if this is enough.
There could be cases where files have been removed and there could be dangling .pyc files left over in some rare cases
There was a problem hiding this comment.
FWIW, I tried to avoid a full clean by doing e.g. 773bffa.
I forgot to mention that I wanted to do this for Linux first to align on the logic, and they (maybe) later do it for Windows, too. But as CI is mostly running on Linux, I don't have any immediate plans to implement this for Windows. |
Codecov Report
@@ Coverage Diff @@
## develop #744 +/- ##
===========================================
- Coverage 77.72% 72.68% -5.05%
===========================================
Files 91 91
Lines 11009 12328 +1319
===========================================
+ Hits 8557 8960 +403
- Misses 2452 3368 +916
Continue to review full report at Codecov.
|
|
LGTM. Thanks! |
|
Somehow there are a few things I am not entirely getting on the overall behavior here. I will dive in it a bit more and eventually submit a new PR/ticket about this |
|
Like, did it break something? Or is it "only" a matter of understanding how it works? |
|
@sschuberth I guess I am not sure how this works and interacts with the SCANCODE_DEV_MODE :P |
|
I was hoping to make that clear in the second paragraph of the top post in this PR :-) |
|
fair enough: let me reread all this first |
|
@sschuberth here are the issues that I can see: If I have a git clone and I ran plain Now with the changes in this PR, a run of It furthermore echoes messages such as There are other possible issues: doing a ./configure after a pull may not be enough and in many cases a ./configure --clean is needed: for instance say a python .py file was renamed: without a clean there may be dangling and orphaned .pyc files left over from before the rename of the corresponding .py file and this can create weird issues I will come up with fixes for this |
Just a quick reply to this one: I disagree here. We already have echo "* Configuring ScanCode for first use...", so I believe if we reconfigure not necessarily because it's the first use, but because there are Git changes, we should say so, too. |
Fair point! So let me come with a new ticket with a clear statement of what we want , can/could/should do here |
|
When I say here, I meant what to do wrt. to running and configuring scancode from different user type standpoints and what should be automated and what should not |
|
@sschuberth since merging is creating some confusion based on @armijnhemel feedback ... may be we could revert these changes in a new PR and apply them anew with some updates? |
|
As you prefer, I'm fine with that! |
|
@sschuberth I reverted the #744 PR commit in develop and pushed the original branch of this PR as https://github.com/nexB/scancode-toolkit/tree/735-769-git-reconfigure Follow up work will take place in in #769 |
Most notably, automatically re-configure if ScanCode is run from a Git working tree and new commits are present. This is useful for running on CI which incrementally fetches ScanCode's code from Git (without applying any local changes).
Note that the new
SCANCODE_GIT_MODEis orthogonal to the existingSCANCODE_DEV_MODE: In development mode the license cache is always checked for consistency, which is not needed when running from a clean Git working tree without any local changes.This fixes issues like the one I've been running into as described at #735.