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

Use reproducible version qualifiers #4569

Merged
merged 1 commit into from
Jan 1, 2025

Conversation

Zlika
Copy link
Contributor

@Zlika Zlika commented Dec 28, 2024

Use reproducible version qualifiers when project.build.outputTimestamp property is set.
This is a follow-up of #4546.

Copy link

github-actions bot commented Dec 28, 2024

Test Results

  606 files    606 suites   4h 8m 20s ⏱️
  434 tests   425 ✅  7 💤 2 ❌
1 302 runs  1 278 ✅ 22 💤 2 ❌

For more details on these failures, see this check.

Results for commit f05034f.

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Member

laeubi commented Dec 29, 2024

This looks useful I just wondering if we probably should handle this in the default timestamp provider instead?

We would then just have the parameter defined as today but read its value here

public Date getTimestamp(MavenSession session, MavenProject project, MojoExecution execution) {
return session.getStartTime();
}

from the mojo execution configuration.

@Zlika
Copy link
Contributor Author

Zlika commented Dec 29, 2024

This looks useful I just wondering if we probably should handle this in the default timestamp provider instead?

We would then just have the parameter defined as today but read its value here

public Date getTimestamp(MavenSession session, MavenProject project, MojoExecution execution) {
return session.getStartTime();
}

from the mojo execution configuration.

I think you're right. I've refactored the PR to only change the DefaultBuildTimestampProvider. What do you think of this?

laeubi
laeubi previously approved these changes Dec 29, 2024
Copy link
Member

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

Testfailure is unrelated

@laeubi
Copy link
Member

laeubi commented Dec 29, 2024

I don't know why but Github indicates you have renamed your previous tests instead of adding a new test. Is this intentional?

@Zlika
Copy link
Contributor Author

Zlika commented Dec 29, 2024

I don't know why but Github indicates you have renamed your previous tests instead of adding a new test. Is this intentional?

I renamed the integration test class from ReproducibleArchiveTimestampTest to ReproducibleBuildTest because I will test here all the reproducibility problems that need to be fixed (other PR will be required for that). Here I added a check to assert that the build version qualifier is the one expected. Adding a new test would require to add another "test project", or to run the tests twice, so I simply added a new check to the existing check. If you think there is a better way to do please tell me.

@laeubi laeubi force-pushed the reproducible-version-qualifiers branch from cbc92ad to bd1ceca Compare December 30, 2024 06:03
@laeubi
Copy link
Member

laeubi commented Dec 30, 2024

@Zlika this looks fine now, can you probably squash all commits into one?

@Zlika
Copy link
Contributor Author

Zlika commented Dec 30, 2024

@Zlika this looks fine now, can you probably squash all commits into one?

Can't you squash the commits directly here when you merge the PR?

@laeubi
Copy link
Member

laeubi commented Dec 30, 2024

I can do it but this often leads to "strange" commits authored by github bot, so if you don't mind, just quash, rebase and force push and I think it can be merged (and backported) right away.

@laeubi laeubi added the backport-to-tycho-4.0.x Can be added to a PR to trigger an automatic backport of the change label Dec 30, 2024
@Zlika Zlika force-pushed the reproducible-version-qualifiers branch 2 times, most recently from cbc92ad to 5ccd6c5 Compare December 30, 2024 11:22
@Zlika
Copy link
Contributor Author

Zlika commented Dec 30, 2024

I can do it but this often leads to "strange" commits authored by github bot, so if you don't mind, just quash, rebase and force push and I think it can be merged (and backported) right away.

Ok, done.Thanks.

Copy link
Member

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

Thanks again!

@laeubi laeubi enabled auto-merge (rebase) December 30, 2024 11:25
@laeubi laeubi disabled auto-merge January 1, 2025 07:31
@laeubi laeubi force-pushed the reproducible-version-qualifiers branch from 5ccd6c5 to f05034f Compare January 1, 2025 07:31
@laeubi laeubi enabled auto-merge (rebase) January 1, 2025 07:31
@laeubi laeubi merged commit 4ca8978 into eclipse-tycho:main Jan 1, 2025
11 of 15 checks passed
@eclipse-tycho-bot
Copy link

💚 All backports created successfully

Status Branch Result
tycho-4.0.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

@Zlika
Copy link
Contributor Author

Zlika commented Jan 1, 2025

Thanks @laeubi for the merge.
I want to propose another PR where I need to create a utility method that will be called from several plugins. I cannot find a good location for this utility method. I tried to put it in tycho-core, but this module is not used by all the plugins. Is there a good location somewhere in the code base for common code that can be used in all the plugins?

@laeubi
Copy link
Member

laeubi commented Jan 2, 2025

We have tycho-api module or tycho-spi, if there is no dependency to any of those it can usually easily be added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-tycho-4.0.x Can be added to a PR to trigger an automatic backport of the change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants