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

Automated dependency update: restore deterministic behavior #875

Merged
merged 20 commits into from
Apr 12, 2023

Conversation

perazz
Copy link
Contributor

@perazz perazz commented Apr 11, 2023

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!


!> 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
Copy link
Contributor Author

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

Comment on lines +215 to +230
"$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

Copy link
Contributor Author

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

Copy link
Member

@everythingfunctional everythingfunctional left a 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.

ci/run_tests.sh Outdated Show resolved Hide resolved
fpm.toml Outdated Show resolved Hide resolved
src/fpm_command_line.f90 Outdated Show resolved Hide resolved
Copy link
Member

@everythingfunctional everythingfunctional left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks.

@urbanjost
Copy link
Contributor

urbanjost commented Apr 11, 2023

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 update appears to be working.

R

@urbanjost
Copy link
Contributor

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.

@perazz
Copy link
Contributor Author

perazz commented Apr 12, 2023

Thank you again @urbanjost, I've tried your example and looks OK on my setup (no open file error after fpm update).
I'll move on and merge this PR for now, so I can release 0.8.1. If you see other repeatably inconsistent behavior, please report and I'll be happy to dig into it further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants