Only update dependencies between cached build and manifest #871

perazz · 2023-04-07T18:59:14Z

Addressing #870.

This update restricts the dependency update to the comparison between a cached build (i.e., cache.toml) and the manifest. This still addresses the issue #837 which originated #843, but solves #870, i.e., dependencies are not replaced/marked for update when duplicates are found down the dependency tree.

@everythingfunctional please let me know if this is the intended behavior.
@awvwgk @certik @urbanjost @minhqdao I would like to also kindly request your opinions especially as far as how this behavior should fit into the upcoming version constraint resolution mechanism.

Happy Easter everyone!

everythingfunctional

Thanks for the quick fix on this. It looks reasonable to me, and I gave it a quick test with my project and seems to fix the problem.

everythingfunctional

I take back my previous statement. On subsequent builds, it still tries to "update" dependencies to the incorrect ones.

perazz · 2023-04-07T20:22:15Z

After some debugging, the git descriptor should be tag but is instead a sha1:

# Dependency ! cached
# - name                     jonquil
# - kind                     git
# Git target
# - URL                      https://github.com/toml-f/jonquil
# - sha1                     v0.2.0
# Dependency ! manifest
# - name                     jonquil
# - kind                     git
# Git target
# - URL                      https://github.com/toml-f/jonquil
# - tag                      v0.2.0

I need to look further, as it seems like the problem may be elsewhere.

perazz · 2023-04-08T05:18:13Z

Thank you @everythingfunctional, I've looked into this a bit further before leaving for Easter. The fpm logic is to

load the cached dependency tree from cache.toml (-> was tree-sorted already)
add manifest dependencies (-> not yet sorted)

With this logic there is no reliable way to check if manifest dependencies should be updated, because we don't know what comes next (they are unsorted: duplicates present). The current implementation is safe in that it gets the right version from fpm.toml, but sometimes it requests for updates more often than necessary.

I propose checking cached dependencies after the manifest tree is first fully built:

build full dependency tree (assume no cache)
search in the cached deps, if some cached dependencies are the same, don't update them

This is the only way the model can fully build the requested (manifest) dependency tree with its constraints, then check what's in the cache to avoid downloading repos again. I believe this logic will also be needed for the version constraint resolution later. the CI is OK but please let me know if this now achieves the right behavior for your cases, plus when I'm back I will start thinking of new test cases

urbanjost · 2023-04-08T13:58:59Z

It should be kept in mind that fpm should be able to operate on a machine with no network connection and except for using git as a substitute for curl or wget be able to work without git.

urbanjost · 2023-04-08T14:59:57Z

Double-checking I am building the right version, as seeing no change. Almost every project with a dependency updates on essentially every fpm command; in several cases this means several minutes per command. Essentially unusable. Wanted to do more 0.8.0 testing but it looks like I build the right PR. An early suggestion to create a module in build/ that optionally could be used for a program that included the git ID when available would be so much nicer as you could tell exactly which version was used and it would change automatically.

perazz · 2023-04-08T16:07:15Z

Thank you @urbanjost for testing - please post as many examples as you can, where you see updates that should not be; I will address as soon as possible. The following test is made with your M_sort module: the dependencies seem to be never rebuilt. Please check that you're using the latest commit of this PR

federico@Federicos-MBP GitHub % git clone https://github.com/urbanjost/M_sort
Cloning into 'M_sort'...
[...]
federico@Federicos-MBP M_sort % fpmx build                    
qsort                                  done.
demo_swap                              done.
demo_sort_heap                         done.
demo_sort_shell.f90                    done.
demo_unique.f90                        done.
demo_swap_any.f90                      done.
demo_sort_quick_compact.f90            done.
demo_sort_quick_rx.f90                 done.
demo_sort_shell                        done.
demo_unique                            done.
demo_swap_any                          done.
demo_sort_quick_compact                done.
demo_sort_quick_rx                     done.
[100%] Project compiled successfully.
federico@Federicos-MBP M_sort % fpmx build
Project is up to date
federico@Federicos-MBP M_sort % fpmx build
Project is up to date
federico@Federicos-MBP M_sort % fpmx update
federico@Federicos-MBP M_sort % fpmx install
<ERROR>Project does not contain any installable targets
STOP 1
federico@Federicos-MBP M_sort % fpmx test
M_msg.F90                              done.
M_journal.f90                          done.
M_verify.F90                           done.
libM_sort.a                            done.
test_suite_M_sort.f90                  done.
qsort                                  done.
demo_swap                              done.
demo_sort_heap                         done.
demo_sort_shell                        done.
demo_unique                            done.
demo_swap_any                          done.
demo_sort_quick_compact                done.
demo_sort_quick_rx                     done.
test_suite_M_sort                      done.
[100%] Project compiled successfully.
check:       sort_shell           SUCCESS : sort string array, ascending

Regarding your suggestion - I will be working on a full dump of the fpm model to JSON/TOML: it will help in this and more complex versioning cases

urbanjost · 2023-04-08T23:40:15Z

a stress test for version 0.7.0 is contained in

wget http://www.urbanjost.altervista.org/REMOVE/urbanjost7.tgz

which I am converting to do basically the same test for 0.8.0; the PR log and ID match but as far as I see so far everyone with a remote dependency rebuilds and "fpm test" causes everything to update even if everything is built with "fpm build".

I cleared everything out, and using a new fpm command I built from PR871 that I changed so "fpm version" shows a captured git ID and see

$ fpm --version
Version:     0.8.0, alpha
Program:     fpm(1)
Description: A Fortran package manager and build system
Home Page:   https://github.com/fortran-lang/fpm
License:     MIT
GitID:       1d5a8282e41dbfefd3cb45f4a50175ec0c021093

and I get an update of everything with every fpm command. Will try again in a few days and see if I am fat-fingering something; will probably not be able to till Tues. though.

perazz · 2023-04-09T05:50:53Z

Shout out @urbanjost, fantastic test suite! You should find a way to standaridize it into fpm testing procedures.

I've reduced it to the cases that do not have issues with implicit typing, enforced since >=0.8.0. Find it here. It seems like unnecessary updates are really never triggered now. This is a summary of the output: I do not experience further git updates ever being triggered

  2 federico@Federicos-MBP urbanjost7 % fpmx build
   3  + mkdir -p build/dependencies    
   4 Initialized empty Git repository in /Users/federico/Downloads/urbanjost7/build/dependencies/orderpack/.git/
[... download all git dependencies]
 279 M_valnth.f90                           done.
[... build all source files]
 345 federico@Federicos-MBP urbanjost7 % fpmx run
 346 <INFO> No executables to run
 347 STOP 0
 348 federico@Federicos-MBP urbanjost7 % fpmx test
 349 test_suite_M_msg.f90                   done.
[... build all tests]
2485 STOP 0       
2486 federico@Federicos-MBP urbanjost7 % fpmx update ! update: no further downloads
2487 federico@Federicos-MBP urbanjost7 % fpmx test ! test: no further downloads
2488 Project is up to date             
2489 check:       mrgref               SUCCESS : real test 1000 values
2490 check:       mrgref               SUCCESS : integer test 1000 values

You can find the full output here; please let me know if you can reproduce it too. To do so, you will have to temporarily remove the new implicit typing policy in fpm.f90, because the manifest's fpm.toml setting does not propagate into the unset dependencies:

            model%packages(i)%name = dependency%name
            associate(features => model%packages(i)%features)
                features%implicit_typing = .true.!dependency%fortran%implicit_typing
                features%implicit_external = .true.!dependency%fortran%implicit_external
                features%source_form = dependency%fortran%source_form
            end associate

everythingfunctional

It looks like this is behaving correctly. The right dependency versions are selected even when rebuilding. However, it reporting that it is re-initialising every dependency on every rebuild is a bit obnoxious. I much prefer a little noise to incorrect behavior though, so I'm fine with accepting this for now and fixing that part later (so long as later isn't too much later).

awvwgk

Thanks for the quick fix. I agree with Brad that we need a follow-up patch for this, but it is important enough to merge quickly and release fpm 0.8.1

awvwgk · 2023-04-10T14:59:48Z

Federico @perazz, the implicit typing is project local on purpose, such that every project on its own can decide on the needed language features.

perazz · 2023-04-11T06:40:40Z

Thank you @awvwgk @everythingfunctional @urbanjost for helping me get this sorted out quickly, glad that it meets the basic requirements now. Brad @everythingfunctional, were you meaning the #Update package-name bloat should only be printed on --verbose? I think it makes a lot of sense, I can open a new issue for that.

urbanjost · 2023-04-11T13:07:32Z

I am seeing 0.8.1 but "fpm update" keeps pulling the remote dependencies.

urbanjs@mercury:/github/M_strings$ fpm --version
Version: 0.8.1, alpha
Program: fpm(1)
Description: A Fortran package manager and build system
Home Page: https://github.com/fortran-lang/fpm
License: MIT
OS Type: Linux
urbanjs@mercury:/github/M_strings$ fpm update

Update: M_msg

Reinitialized existing Git repository in /home/urbanjs/venus/V600/github/M_strings/build/dependencies/M_msg/.git/
remote: Enumerating objects: 275, done.
remote: Counting objects: 100% (275/275), done.
remote: Compressing objects: 100% (140/140), done.
remote: Total 275 (delta 147), reused 221 (delta 131), pack-reused 0
Receiving objects: 100% (275/275), 1.14 MiB | 4.28 MiB/s, done.
Resolving deltas: 100% (147/147), done.
From https://github.com/urbanjost/M_msg

branch HEAD -> FETCH_HEAD
urbanjs@mercury:~/github/M_strings$ fpm update

Update: M_msg

Reinitialized existing Git repository in /home/urbanjs/venus/V600/github/M_strings/build/dependencies/M_msg/.git/
remote: Enumerating objects: 275, done.
remote: Counting objects: 100% (275/275), done.
remote: Compressing objects: 100% (140/140), done.
remote: Total 275 (delta 147), reused 221 (delta 131), pack-reused 0
Receiving objects: 100% (275/275), 1.14 MiB | 2.71 MiB/s, done.
Resolving deltas: 100% (147/147), done.
From https://github.com/urbanjost/M_msg

branch HEAD -> FETCH_HEAD

perazz added 4 commits April 7, 2023 20:02

only update dependencies that were cached

68c13cf

add test to CI

d241617

update dependency tree test

b3ac9c2

simplify CI

9171051

everythingfunctional approved these changes Apr 7, 2023

View reviewed changes

everythingfunctional requested changes Apr 7, 2023

View reviewed changes

perazz added 2 commits April 8, 2023 07:02

build separate cache and project trees, then compare dependencies

a96c056

update cached deps test

8c69707

improve comment

22e1324

perazz requested a review from everythingfunctional April 8, 2023 05:20

test dependency that should not be updated

1d5a828

everythingfunctional approved these changes Apr 10, 2023

View reviewed changes

awvwgk approved these changes Apr 10, 2023

View reviewed changes

awvwgk merged commit 28e1e5f into fortran-lang:main Apr 11, 2023
9 checks passed

perazz mentioned this pull request Apr 11, 2023

Print dependency update messages on verbosity>0 #873

Merged

perazz deleted the fix_dependency_cascade branch April 11, 2023 11:09

perazz mentioned this pull request Apr 12, 2023

Dependency Version Update Breaks Expected Behavior #870

Closed

perazz mentioned this pull request Jun 17, 2023

Force user to specify single version of a dependency if it is requested by multiple packages #867

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only update dependencies between cached build and manifest #871

Only update dependencies between cached build and manifest #871

perazz commented Apr 7, 2023 •

edited

everythingfunctional left a comment

everythingfunctional left a comment

perazz commented Apr 7, 2023

perazz commented Apr 8, 2023 •

edited

urbanjost commented Apr 8, 2023

urbanjost commented Apr 8, 2023

perazz commented Apr 8, 2023 •

edited

urbanjost commented Apr 8, 2023

perazz commented Apr 9, 2023 •

edited

everythingfunctional left a comment

awvwgk left a comment

awvwgk commented Apr 10, 2023

perazz commented Apr 11, 2023

urbanjost commented Apr 11, 2023

Only update dependencies between cached build and manifest #871

Only update dependencies between cached build and manifest #871

Conversation

perazz commented Apr 7, 2023 • edited

everythingfunctional left a comment

Choose a reason for hiding this comment

everythingfunctional left a comment

Choose a reason for hiding this comment

perazz commented Apr 7, 2023

perazz commented Apr 8, 2023 • edited

urbanjost commented Apr 8, 2023

urbanjost commented Apr 8, 2023

perazz commented Apr 8, 2023 • edited

urbanjost commented Apr 8, 2023

perazz commented Apr 9, 2023 • edited

everythingfunctional left a comment

Choose a reason for hiding this comment

awvwgk left a comment

Choose a reason for hiding this comment

awvwgk commented Apr 10, 2023

perazz commented Apr 11, 2023

urbanjost commented Apr 11, 2023

Update: M_msg

Update: M_msg

perazz commented Apr 7, 2023 •

edited

perazz commented Apr 8, 2023 •

edited

perazz commented Apr 8, 2023 •

edited

perazz commented Apr 9, 2023 •

edited