Only update dependencies between cached build and manifest (#871)

* only update dependencies that were cached

* add test to CI

* update dependency tree test

* simplify CI

* build separate `cache` and `project` trees, then compare dependencies

* update cached deps test

* improve comment

* test dependency that should not be updated

Author: perazz

ci/run_tests.sh

@@ -201,6 +201,10 @@ EXIT_CODE=0
 test $EXIT_CODE -eq 1
 popd
 
+# test dependency priority
+pushd dependency_priority
+"$fpm" run 
+popd
 
 # Cleanup
 rm -rf ./*/build

example_packages/dependency_priority/README.md

@@ -0,0 +1,4 @@
+# dependency_tree
+Check dependency tree cascade. "Standard" fpm dependencies feature that the highest-priority one wins
+(i.e., top-level dependencies in the manifest, or the first time it's found down the dependency tree)
+Check this behavior is confirmed.

example_packages/dependency_priority/app/main.f90

@@ -0,0 +1,14 @@
+program main
+  use tomlf_version, only: tomlf_version_string
+  implicit none
+
+  print *, 'using version =',tomlf_version_string
+  print *, 'should be     =0.3.1'
+
+  if (tomlf_version_string=="0.3.1") then 
+     stop 0
+  else
+     stop 1
+  endif
+
+end program main

example_packages/dependency_priority/fpm.toml

@@ -0,0 +1,12 @@
+name = "dependency_tree"
+version = "0.1.0"
+[build]
+auto-executables=true
+[dependencies]
+# Request toml-f v0.3.1. 
+toml-f.git = "https://github.com/toml-f/toml-f"
+toml-f.tag = "v0.3.1"
+# jonquil 0.2.0 requires toml-f v0.4.0. 
+# Because 0.4.0 is a derived dependency, it should not be used
+jonquil.git = "https://github.com/toml-f/jonquil"
+jonquil.tag = "v0.2.0"

src/fpm/dependency.f90

@@ -93,6 +93,8 @@ module fpm_dependency
     logical :: done = .false.
     !> Dependency should be updated
     logical :: update = .false.
+    !> Dependency was loaded from a cache
+    logical :: cached = .false.
   contains
     !> Update dependency from project manifest.
     procedure :: register
@@ -284,12 +286,9 @@ subroutine add_project(self, package, error)
     type(error_t), allocatable, intent(out) :: error
 
     type(dependency_config_t) :: dependency
+    type(dependency_tree_t) :: cached
     character(len=*), parameter :: root = '.'
-
-    if (allocated(self%cache)) then
-      call self%load(self%cache, error)
-      if (allocated(error)) return
-    end if
+    integer :: id
 
     if (.not. exists(self%dep_dir)) then
       call mkdir(self%dep_dir)
@@ -309,6 +308,20 @@ subroutine add_project(self, package, error)
     call self%add(package, root, .true., error)
     if (allocated(error)) return
 
+    ! After resolving all dependencies, check if we have cached ones to avoid updates
+    if (allocated(self%cache)) then
+      call new_dependency_tree(cached, cache=self%cache)
+      call cached%load(self%cache, error)
+      if (allocated(error)) return
+
+      ! Skip root node
+      do id=2,cached%ndep
+          cached%dep(id)%cached = .true.
+          call self%add(cached%dep(id), error)
+          if (allocated(error)) return
+      end do
+    end if
+
     ! Now decent into the dependency tree, level for level
     do while (.not. self%finished())
       call self%resolve(root, error)
@@ -423,11 +436,19 @@ subroutine add_dependency_node(self, dependency, error)
       ! Check if it needs to be updated
       id = self%find(dependency%name)
 
-      ! Ensure an update is requested whenever the dependency has changed
-      if (dependency_has_changed(self%dep(id), dependency)) then
-        write (self%unit, out_fmt) "Dependency change detected:", dependency%name
-        self%dep(id) = dependency
-        self%dep(id)%update = .true.
+      ! If this dependency was in the cache, and we're now requesting a different version
+      ! in the manifest, ensure it is marked for update. Otherwise, if we're just querying
+      ! the same dependency from a lower branch of the dependency tree, the existing one from
+      ! the manifest has priority
+      if (dependency%cached) then
+        if (dependency_has_changed(dependency, self%dep(id))) then
+           write (self%unit, out_fmt) "Dependency change detected:", dependency%name
+           self%dep(id)%update = .true.
+        else
+           ! Store the cached one
+           self%dep(id) = dependency
+           self%dep(id)%update = .false.
+        endif
       end if
     else
       ! New dependency: add from scratch
@@ -1161,26 +1182,26 @@ pure subroutine resize_dependency_node(var, n)
   end subroutine resize_dependency_node
 
   !> Check if a dependency node has changed
-  logical function dependency_has_changed(this, that) result(has_changed)
+  logical function dependency_has_changed(cached, manifest) result(has_changed)
     !> Two instances of the same dependency to be compared
-    type(dependency_node_t), intent(in) :: this, that
+    type(dependency_node_t), intent(in) :: cached, manifest
 
     has_changed = .true.
 
     !> All the following entities must be equal for the dependency to not have changed
-    if (manifest_has_changed(this, that)) return
+    if (manifest_has_changed(cached=cached, manifest=manifest)) return
 
     !> 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(this%version) .and. allocated(that%version)) then
-      if (this%version /= that%version) return
+    if (allocated(cached%version) .and. allocated(manifest%version)) then
+      if (cached%version /= manifest%version) return
     end if
-    if (allocated(this%revision) .and. allocated(that%revision)) then
-      if (this%revision /= that%revision) return
+    if (allocated(cached%revision) .and. allocated(manifest%revision)) then
+      if (cached%revision /= manifest%revision) return
     end if
-    if (allocated(this%proj_dir) .and. allocated(that%proj_dir)) then
-      if (this%proj_dir /= that%proj_dir) return
+    if (allocated(cached%proj_dir) .and. allocated(manifest%proj_dir)) then
+      if (cached%proj_dir /= manifest%proj_dir) return
     end if
 
     !> All checks passed: the two dependencies have no differences

src/fpm/git.f90

@@ -8,6 +8,7 @@ module fpm_git
     public :: git_target_default, git_target_branch, git_target_tag, &
         & git_target_revision
     public :: git_revision
+    public :: git_matches_manifest
     public :: operator(==)
 
 
@@ -36,7 +37,7 @@ module fpm_git
     type :: git_target_t
 
         !> Kind of the git target
-        integer, private :: descriptor = git_descriptor%default
+        integer :: descriptor = git_descriptor%default
 
         !> Target URL of the git repository
         character(len=:), allocatable :: url
@@ -145,6 +146,23 @@ logical function git_target_eq(this,that) result(is_equal)
 
     end function git_target_eq
 
+    !> Check that a cached dependency matches a manifest request
+    logical function git_matches_manifest(cached,manifest)
+
+        !> Two input git targets
+        type(git_target_t), intent(in) :: cached,manifest
+
+        git_matches_manifest = cached%url == manifest%url
+        if (.not.git_matches_manifest) return
+
+        !> The manifest dependency only contains partial information (what's requested),
+        !> while the cached dependency always stores a commit hash because it's built
+        !> after the repo is available (saved as git_descriptor%revision==revision).
+        !> So, comparing against the descriptor is not reliable
+        git_matches_manifest = cached%object == manifest%object
+
+    end function git_matches_manifest
+
 
     subroutine checkout(self, local_path, error)
 

src/fpm/manifest/dependency.f90

@@ -25,7 +25,7 @@
 module fpm_manifest_dependency
     use fpm_error, only: error_t, syntax_error
     use fpm_git, only: git_target_t, git_target_tag, git_target_branch, &
-        & git_target_revision, git_target_default, operator(==)
+        & git_target_revision, git_target_default, operator(==), git_matches_manifest
     use fpm_toml, only: toml_table, toml_key, toml_stat, get_value, check_keys
     use fpm_filesystem, only: windows_path
     use fpm_environment, only: get_os_type, OS_WINDOWS
@@ -274,19 +274,17 @@ subroutine info(self, unit, verbosity)
     end subroutine info
 
     !> Check if two dependency configurations are different
-    logical function manifest_has_changed(this, that) result(has_changed)
+    logical function manifest_has_changed(cached, manifest) result(has_changed)
 
         !> Two instances of the dependency configuration
-        class(dependency_config_t), intent(in) :: this, that
+        class(dependency_config_t), intent(in) :: cached, manifest
 
         has_changed = .true.
 
         !> Perform all checks
-        if (this%name/=that%name) return
-        if (this%path/=that%path) return
-        if (allocated(this%git).neqv.allocated(that%git)) return
-        if (allocated(this%git)) then
-            if (.not.(this%git==that%git)) return
+        if (allocated(cached%git).neqv.allocated(manifest%git)) return
+        if (allocated(cached%git)) then
+            if (.not.git_matches_manifest(cached%git,manifest%git)) return
         end if
 
         !> All checks passed! The two instances are equal

test/fpm_test/test_package_dependencies.f90

@@ -45,6 +45,7 @@ subroutine collect_package_dependencies(tests)
         & new_unittest("status-after-load", test_status), &
         & new_unittest("add-dependencies", test_add_dependencies), &
         & new_unittest("update-dependencies", test_update_dependencies), &
+        & new_unittest("do-not-update-dependencies", test_non_updated_dependencies), &
         & new_unittest("registry-dir-not-found", registry_dir_not_found, should_fail=.true.), &
         & new_unittest("no-versions-in-registry", no_versions_in_registry, should_fail=.true.), &
      & new_unittest("local-registry-specified-version-not-found", local_registry_specified_version_not_found, should_fail=.true.), &
@@ -254,15 +255,15 @@ subroutine test_add_dependencies(error)
 
   end subroutine test_add_dependencies
 
-  subroutine test_update_dependencies(error)
+  subroutine test_non_updated_dependencies(error)
 
     !> Error handling
     type(error_t), allocatable, intent(out) :: error
 
     type(toml_table) :: cache, manifest
     type(toml_table), pointer :: ptr
     type(toml_key), allocatable :: list(:)
-    type(dependency_tree_t) :: deps, cached_deps
+    type(dependency_tree_t) :: cached, manifest_deps
     integer :: ii
 
     ! Create a dummy cache
@@ -283,11 +284,99 @@ subroutine test_update_dependencies(error)
     call set_value(ptr, "proj-dir", "fpm-tmp1-dir")
 
     ! Load into a dependency tree
-    call new_dependency_tree(cached_deps)
-    call cached_deps%load(cache, error)
+    call new_dependency_tree(cached)
+    call cached%load(cache, error)
+    if (allocated(error)) return
+    ! Mark all dependencies as "cached"
+    do ii=1,cached%ndep
+        cached%dep(ii)%cached = .true.
+    end do
     call cache%destroy()
+
+    ! Create a dummy manifest, with different version
+    manifest = toml_table()
+    call add_table(manifest, "dep1", ptr)
+    call set_value(ptr, "version", "1.1.1")
+    call set_value(ptr, "proj-dir", "fpm-tmp1-dir")
+    call add_table(manifest, "dep2", ptr)
+    call set_value(ptr, "git", "https://gitlab.com/fortran-lang/lin4")
+    call set_value(ptr, "rev", "c0ffee")
+    call set_value(ptr, "proj-dir", "fpm-tmp1-dir")
+    call add_table(manifest, "dep3", ptr)
+    call set_value(ptr, "git", "https://gitlab.com/fortran-lang/pkg3")
+    call set_value(ptr, "rev", "t4a")
+    call set_value(ptr, "proj-dir", "fpm-tmp1-dir")
+
+    ! Load dependencies from manifest
+    call new_dependency_tree(manifest_deps)
+    call manifest_deps%load(manifest, error)
+    call manifest%destroy()
     if (allocated(error)) return
 
+    ! Add cached dependencies afterwards; will flag those that need udpate
+    do ii=1,cached%ndep
+        cached%dep(ii)%cached = .true.
+        call manifest_deps%add(cached%dep(ii), error)
+        if (allocated(error)) return
+    end do
+
+    ! Test that dependencies 1-2 are flagged as "update"
+    if (.not. manifest_deps%dep(1)%update) then
+      call test_failed(error, "Updated dependency (different version) not detected")
+      return
+    end if
+    if (.not. manifest_deps%dep(2)%update) then
+      call test_failed(error, "Updated dependency (git address) not detected")
+      return
+    end if
+
+
+    ! Test that dependency 3 is flagged as "not update"
+    if (manifest_deps%dep(3)%update) then
+      call test_failed(error, "Updated dependency (git rev) detected, should not be")
+      return
+    end if
+
+  end subroutine test_non_updated_dependencies
+
+  subroutine test_update_dependencies(error)
+
+    !> Error handling
+    type(error_t), allocatable, intent(out) :: error
+
+    type(toml_table) :: cache, manifest
+    type(toml_table), pointer :: ptr
+    type(toml_key), allocatable :: list(:)
+    type(dependency_tree_t) :: cached, manifest_deps
+    integer :: ii
+
+    ! Create a dummy cache
+    cache = toml_table()
+    call add_table(cache, "dep1", ptr)
+    call set_value(ptr, "version", "1.1.0")
+    call set_value(ptr, "proj-dir", "fpm-tmp1-dir")
+    call add_table(cache, "dep2", ptr)
+    call set_value(ptr, "git", "https://gitlab.com/fortran-lang/lin2")
+    call set_value(ptr, "rev", "c0ffee")
+    call set_value(ptr, "proj-dir", "fpm-tmp1-dir")
+    call add_table(cache, "dep3", ptr)
+    call set_value(ptr, "git", "https://gitlab.com/fortran-lang/pkg3")
+    call set_value(ptr, "rev", "t4a")
+    call set_value(ptr, "proj-dir", "fpm-tmp1-dir")
+    call add_table(cache, "dep4", ptr)
+    call set_value(ptr, "version", "1.0.0")
+    call set_value(ptr, "proj-dir", "fpm-tmp1-dir")
+
+    ! Load into a dependency tree
+    call new_dependency_tree(cached)
+    call cached%load(cache, error)
+    if (allocated(error)) return
+    ! Mark all dependencies as "cached"
+    do ii=1,cached%ndep
+        cached%dep(ii)%cached = .true.
+    end do
+    call cache%destroy()
+
     ! Create a dummy manifest, with different version
     manifest = toml_table()
     call add_table(manifest, "dep1", ptr)
@@ -303,27 +392,28 @@ subroutine test_update_dependencies(error)
     call set_value(ptr, "proj-dir", "fpm-tmp1-dir")
 
     ! Load dependencies from manifest
-    call new_dependency_tree(deps)
-    call deps%load(manifest, error)
+    call new_dependency_tree(manifest_deps)
+    call manifest_deps%load(manifest, error)
     call manifest%destroy()
     if (allocated(error)) return
 
-    ! Add manifest dependencies
-    do ii = 1, cached_deps%ndep
-      call deps%add(cached_deps%dep(ii), error)
-      if (allocated(error)) return
+    ! Add cached dependencies afterwards; will flag those that need udpate
+    do ii=1,cached%ndep
+        cached%dep(ii)%cached = .true.
+        call manifest_deps%add(cached%dep(ii), error)
+        if (allocated(error)) return
     end do
 
     ! Test that all dependencies are flagged as "update"
-    if (.not. deps%dep(1)%update) then
+    if (.not. manifest_deps%dep(1)%update) then
       call test_failed(error, "Updated dependency (different version) not detected")
       return
     end if
-    if (.not. deps%dep(2)%update) then
+    if (.not. manifest_deps%dep(2)%update) then
       call test_failed(error, "Updated dependency (git address) not detected")
       return
     end if
-    if (.not. deps%dep(3)%update) then
+    if (.not. manifest_deps%dep(3)%update) then
       call test_failed(error, "Updated dependency (git rev) not detected")
       return
     end if