Automated dependency update: restore deterministic behavior (#875)

Fix undefined behavior from 1f604b4, was triggering inconsistent module updates

Author: perazz

ci/run_tests.sh

@@ -203,7 +203,28 @@ popd
 
 # test dependency priority
 pushd dependency_priority
-"$fpm" run 
+
+# first build should run OK
+EXIT_CODE=0
+"$fpm" run || EXIT_CODE=$?
+test $EXIT_CODE -eq 0
+
+"$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
+
 popd
 
 # Cleanup

src/fpm/cmd/update.f90

@@ -40,6 +40,13 @@ subroutine cmd_update(settings)
     call deps%add(package, error)
     call handle_error(error)
 
+    ! Force-update all dependencies if `--clean`
+    if (settings%clean) then
+        do ii = 1, deps%ndep
+            deps%dep(ii)%update = .true.
+        end do
+    end if
+
     if (settings%fetch_only) return
 
     if (size(settings%name) == 0) then

src/fpm/dependency.f90

@@ -310,7 +310,7 @@ subroutine add_project(self, package, error)
 
     ! 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 new_dependency_tree(cached, verbosity=self%verbosity,cache=self%cache)
       call cached%load(self%cache, error)
       if (allocated(error)) return
 
@@ -441,7 +441,7 @@ subroutine add_dependency_node(self, dependency, error)
       ! 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
+        if (dependency_has_changed(dependency, self%dep(id), self%verbosity, self%unit)) then
            if (self%verbosity>0) write (self%unit, out_fmt) "Dependency change detected:", dependency%name
            self%dep(id)%update = .true.
         else
@@ -454,6 +454,7 @@ subroutine add_dependency_node(self, dependency, error)
       ! New dependency: add from scratch
       self%ndep = self%ndep + 1
       self%dep(self%ndep) = dependency
+      self%dep(self%ndep)%update = .false.
     end if
 
   end subroutine add_dependency_node
@@ -576,6 +577,7 @@ subroutine resolve_dependency(self, dependency, global_settings, root, error)
 
     if (dependency%done) return
 
+    fetch = .false.
     if (allocated(dependency%proj_dir)) then
       proj_dir = dependency%proj_dir
     else if (allocated(dependency%path)) then
@@ -947,16 +949,12 @@ subroutine register(self, package, root, fetch, revision, error)
     if (allocated(self%git) .and. present(revision)) then
       self%revision = revision
       if (.not. fetch) then
-        ! git object is HEAD always allows an update
-        update = .not. allocated(self%git%object)
-        if (.not. update) then
-          ! allow update in case the revision does not match the requested object
-          update = revision /= self%git%object
-        end if
+        ! Change in revision ID was checked already. Only update if ALL git information is missing
+        update = .not. allocated(self%git%url)
       end if
     end if
 
-    self%update = update
+    if (update) self%update = update
     self%done = .true.
 
   end subroutine register
@@ -1182,26 +1180,44 @@ pure subroutine resize_dependency_node(var, n)
   end subroutine resize_dependency_node
 
   !> Check if a dependency node has changed
-  logical function dependency_has_changed(cached, manifest) result(has_changed)
+  logical function dependency_has_changed(cached, manifest, verbosity, iunit) result(has_changed)
     !> Two instances of the same dependency to be compared
     type(dependency_node_t), intent(in) :: cached, manifest
 
+    !> Log verbosity
+    integer, intent(in) :: verbosity, iunit
+
     has_changed = .true.
 
     !> All the following entities must be equal for the dependency to not have changed
-    if (manifest_has_changed(cached=cached, manifest=manifest)) return
+    if (manifest_has_changed(cached=cached, manifest=manifest, verbosity=verbosity, iunit=iunit)) 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(cached%version) .and. allocated(manifest%version)) then
-      if (cached%version /= manifest%version) return
+      if (cached%version /= manifest%version) then
+         if (verbosity>1) write(iunit,out_fmt) "VERSION has changed: "//cached%version%s()//" vs. "//manifest%version%s()
+         return
+      endif
+    else
+       if (verbosity>1) write(iunit,out_fmt) "VERSION has changed presence "
     end if
     if (allocated(cached%revision) .and. allocated(manifest%revision)) then
-      if (cached%revision /= manifest%revision) return
+      if (cached%revision /= manifest%revision) then
+        if (verbosity>1) write(iunit,out_fmt) "REVISION has changed: "//cached%revision//" vs. "//manifest%revision
+        return
+      endif
+    else
+      if (verbosity>1) write(iunit,out_fmt) "REVISION has changed presence "
     end if
     if (allocated(cached%proj_dir) .and. allocated(manifest%proj_dir)) then
-      if (cached%proj_dir /= manifest%proj_dir) return
+      if (cached%proj_dir /= manifest%proj_dir) then
+        if (verbosity>1) write(iunit,out_fmt) "PROJECT DIR has changed: "//cached%proj_dir//" vs. "//manifest%proj_dir
+        return
+      endif
+    else
+      if (verbosity>1) write(iunit,out_fmt) "PROJECT DIR has changed presence "
     end if
 
     !> All checks passed: the two dependencies have no differences

src/fpm/git.f90

@@ -60,6 +60,9 @@ module fpm_git
         module procedure git_target_eq
     end interface
 
+    !> Common output format for writing to the command line
+    character(len=*), parameter :: out_fmt = '("#", *(1x, g0))'
+
 contains
 
 
@@ -147,19 +150,27 @@ 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)
+    logical function git_matches_manifest(cached,manifest,verbosity,iunit)
 
         !> Two input git targets
         type(git_target_t), intent(in) :: cached,manifest
 
+        integer, intent(in) :: verbosity,iunit
+
         git_matches_manifest = cached%url == manifest%url
-        if (.not.git_matches_manifest) return
+        if (.not.git_matches_manifest) then
+            if (verbosity>1) write(iunit,out_fmt) "GIT URL has changed: ",cached%url," vs. ", manifest%url
+            return
+        endif
 
         !> 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
+        if (.not.git_matches_manifest) then
+            if (verbosity>1) write(iunit,out_fmt) "GIT OBJECT has changed: ",cached%object," vs. ", manifest%object
+        end if
 
     end function git_matches_manifest
 

src/fpm/manifest/dependency.f90

@@ -63,6 +63,9 @@ module fpm_manifest_dependency
 
     end type dependency_config_t
 
+    !> Common output format for writing to the command line
+    character(len=*), parameter :: out_fmt = '("#", *(1x, g0))'
+
 contains
 
     !> Construct a new dependency configuration from a TOML data structure
@@ -274,17 +277,23 @@ subroutine info(self, unit, verbosity)
     end subroutine info
 
     !> Check if two dependency configurations are different
-    logical function manifest_has_changed(cached, manifest) result(has_changed)
+    logical function manifest_has_changed(cached, manifest, verbosity, iunit) result(has_changed)
 
         !> Two instances of the dependency configuration
         class(dependency_config_t), intent(in) :: cached, manifest
 
+        !> Log verbosity
+        integer, intent(in) :: verbosity, iunit
+
         has_changed = .true.
 
         !> Perform all checks
-        if (allocated(cached%git).neqv.allocated(manifest%git)) return
+        if (allocated(cached%git).neqv.allocated(manifest%git)) then
+            if (verbosity>1) write(iunit,out_fmt) "GIT presence has changed. "
+            return
+        endif
         if (allocated(cached%git)) then
-            if (.not.git_matches_manifest(cached%git,manifest%git)) return
+            if (.not.git_matches_manifest(cached%git,manifest%git,verbosity,iunit)) return
         end if
 
         !> All checks passed! The two instances are equal