Skip to content

fix build failure related to stale versions in the cache #1449

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 25, 2023
Merged

fix build failure related to stale versions in the cache #1449

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14f9d00
fix
wind57 Sep 22, 2023
f3e0aad
fix
wind57 Sep 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/composites/build-integration-tests-project/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@ runs:

cd spring-cloud-kubernetes-integration-tests
# build the images, but dont run the tests
.././mvnw -T 1C clean install -DskipTests
.././mvnw -T 1C clean install -nsu -DskipTests
Copy link
Contributor Author

@wind57 wind57 Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this is a fix that affects us in a totally different PR, but I want to have it in 3.0.x, so that you merge it to main and I take it from main.

-nsu means no-snapshot-update and it has bitten me in a PR where I renamed classes.

To be more precise, there is an integration test that looks like this:

public class Fabric8DiscoveryController {

	private final Fabric8KubernetesDiscoveryClient discoveryClient;

}

Now, in the branch where I am working, Fabric8KubernetesDiscoveryClient exists, but in main it is called: KubernetesDiscoveryClient.

What happens is that the version that contains KubernetesDiscoveryClient is already published in the spring repo as a SNAPSHOT, so without -nsu here, it will go and download the version that will fail compilation. We need Fabric8KubernetesDiscoveryClient, but get KubernetesDiscoveryClient.

I hope it makes sense.

P.S. I don't understand why it did not happen until now, because that branch had a lot of successful builds, I assume you only recently started to publish 3.1.0-SNAPSHOT.

Copy link
Contributor

@ryanjbaxter ryanjbaxter Sep 22, 2023

Choose a reason for hiding this comment

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

Are you saying its pulling down a snapshot version from the wrong branch?

Copy link
Contributor Author

@wind57 wind57 Sep 22, 2023

Choose a reason for hiding this comment

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

yes, basically I need my own version that I build during the build phase, but without -nsu it's pulling the version that exists in spring snapshot repos.

Copy link
Contributor

@ryanjbaxter ryanjbaxter Sep 22, 2023

Choose a reason for hiding this comment

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

Well then there is something else that is wrong then because it at the very least it should be pulling the version for the branch being built not a different version.

Copy link
Contributor Author

@wind57 wind57 Sep 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

first I have to apologize: the fact that I did not understand 100% of what happens, rushed me into a PR that would have only partially solve the issue. I spent some time into figuring it out and now I can present my case.

Here it is:

  1. we use a cache of maven .m2 directory for obvious reasons.
  2. before we build anything, we download that cache.

Now suppose such a sequence of events:

  • in our cache we have a version of libX-3.1.0-SNAPSHOT that contains a class called: Fabric8KubernetesDiscoveryClient that in turn is needed by libY-3.1.0-SNAPSHOT.
  • because we cache such entries, in our github cache, we will have libX-3.1.0-SNAPSHOT, but it will have such a maven-metadata-local.xml
<?xml version="1.0" encoding="UTF-8"?>
<metadata modelVersion="1.1.0">
  <groupId>.....</groupId>
  <artifactId>..</artifactId>
  <versioning>
    <lastUpdated>20230920045240</lastUpdated>
    <snapshot>
      <localCopy>true</localCopy>
    </snapshot>
    <snapshotVersions>
      <snapshotVersion>
        <classifier>sources</classifier>
        <extension>jar</extension>
        <value>3.1.0-SNAPSHOT</value>
        <updated>20230920045240</updated>
      </snapshotVersion>
      <snapshotVersion>
        <classifier>javadoc</classifier>
        <extension>jar</extension>
        <value>3.1.0-SNAPSHOT</value>
        <updated>20230920045240</updated>
      </snapshotVersion>
      <snapshotVersion>
        <extension>jar</extension>
        <value>3.1.0-SNAPSHOT</value>
        <updated>20230920045240</updated>
      </snapshotVersion>
      <snapshotVersion>
        <extension>pom</extension>
        <value>3.1.0-SNAPSHOT</value>
        <updated>20230920045240</updated>
      </snapshotVersion>
    </snapshotVersions>
  </versioning>
  <version>3.1.0-SNAPSHOT</version>
</metadata>

This file decides if we should update our snapshots or not. Notice how lastUpdated is : 20230920045240, or simpler: 2023/09/20. So this is when we placed this file in our cache, and every time we restore the cache, this is what maven is going to know about it.

Now the problem is that this dependency might be too old, the actual dependency stored in spring snapshot repo might be newer. If it is indeed newer, then we will try to download it and this is happens in our case.

  • local cache has : libX-3.1.0-SNAPSHOT with Fabric8KubernetesDiscoveryClient
  • spring repo snapshots libX-3.1.0-SNAPSHOT without Fabric8KubernetesDiscoveryClient

spring repo dependency is newer and this is what turns out being used, compilation failing as such.

What this PR does is to build the project after the cache is downloaded, in this manner maven will use the locally build snapshot, always.

Copy link
Contributor

@ryanjbaxter ryanjbaxter Sep 25, 2023

Choose a reason for hiding this comment

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

So it wasn't that we were pulling the wrong version of the dependencies, its that the cached version was using what was coming from spring snapshots and not what was built by the build?

Copy link
Contributor Author

@wind57 wind57 Sep 25, 2023

Choose a reason for hiding this comment

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

exactly.

cd ..
4 changes: 2 additions & 2 deletions .github/workflows/composites/maven-build-with-dry-run-for-tests/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@ runs:
using: "composite"
steps:

- name: run 'package' on the project
- name: run 'install' on the project
shell: bash
run: |
./mvnw install -B \
-Dskip.build.image=true \
-DskipTests -DskipITs \
-T 1C -q
-T 1C -U -q
Copy link
Contributor

@ryanjbaxter ryanjbaxter Sep 25, 2023

Choose a reason for hiding this comment

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

Doesn't -U tell maven to update dependencies?

Copy link
Contributor Author

@wind57 wind57 Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yes, but this is what we want here, since we already restored the cache at this point. So this just updates potentially libraries that are not part of our maven reactor, which is good. it's like we update spring-boot-XXX-SNAPSHOT in this step, but not spring-cloud-kubernetes-commons-SNAPSHOT, if it makes sense

Copy link
Contributor

@ryanjbaxter ryanjbaxter Sep 25, 2023

Choose a reason for hiding this comment

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

Makes sense. I will just point out there is the potential...due to bad timing that we could finish the build locally and install those snapshots but inbetween that time and this command we could publish new snapshots to repo.spring.io and pull down those as well (for k8s). Although the chance of that is quite small and I am not sure there is anything we can really do about it.


- name: find all classpath entries
shell: bash
Expand Down