-
Notifications
You must be signed in to change notification settings - Fork 101
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
Automated dependency update: restore deterministic behavior #875
Conversation
|
||
!> For now, only perform the following checks if both are available. A dependency in cache.toml | ||
!> will always have this metadata; a dependency from fpm.toml which has not been fetched yet | ||
!> may not have it | ||
if (allocated(cached%version) .and. allocated(manifest%version)) then | ||
if (cached%version /= manifest%version) return | ||
if (cached%version /= manifest%version) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Output bloatware only if verbosity > 1
"$fpm" build --verbose | ||
|
||
# Build again, should update nothing | ||
"$fpm" build --verbose > build.log | ||
if [[ -n "$(grep Update build.log)" ]]; then | ||
echo "Some dependencies were updated that should not be"; | ||
exit 1; | ||
fi | ||
|
||
# Request update --clean, should update all dependencies | ||
"$fpm" update --clean --verbose > update.log | ||
if [[ -z "$(grep Update update.log)" ]]; then | ||
echo "No updated dependencies after 'fpm update --clean'"; | ||
exit 1; | ||
fi | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
live-test that a second build does not update anything, and then update --clean
updates everything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few procedural type comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks.
Have not reviewed the changes yet, but looks better so far running QA tests. So far only new bug seems to be cd /tmp
git pull https://github.com/urbanjost/M_strings.git
cd M_strings
fpm clean --all
fpm test
fpm update
<ERROR>*fileopen*:build/.gitignore:cannot overwrite existing file, unit -129, file /home/urbanjs/venus/V600/github/M_strings/build/.gitignore
3 which passed with fpm 0.7.0 last time I tried. After updating several of the test packages the urbanjost7 test project builds with gfortran on Linux; although some of the packages now require 0.8.0; but as far as blackbox testing everything except R |
A minor problem was that the gfortran switch caused having to rearrange M_display so functions used in declarations required being declared before they were used in the same module, which I have never seen a compiler require with or without implicit none when all are contained in the same module. It allowed procedures declared later to be called in the body of the procedure but not in the declarations. Looking into whether that is a common extension or a gfortran bug but I think that is a rare usage so far in Fortran codes; but that was the only module out of dozens where I had to rearrange things. I do not consider that anything resolvable by fpm(1) easily and probably a compiler bug so it is just an FYI; I have tried a few others packages and have not seen this show up anywhere else so far. |
Thank you again @urbanjost, I've tried your example and looks OK on my setup (no open file error after |
Before releasing 0.8.1, I'd kindly ask @awvwgk @urbanjost @everythingfunctional @minhqdao to validate and merge this PR.
This PR was made necessary by the inconsistencies found by @urbanjost: all checks passed on CI scripts and local testing on my machine (macOS, gfortran-12); unnecessary dependency updates were still experienced on @urbanjost's systems.
I've tracked down undefined behavior due to uninitialized variable at commit 1f604b4, see discussion in #874.
All checks OK on my side now, please confirm & merge so we can release 0.8.1 no later than tomorrow, if possible.
Thank you all for the excellent debugging work!