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

Tezos/Octez: Mark old packages as unavailable #23649

Merged

Conversation

killian-delarue
Copy link
Contributor

This pull request makes the old Octez pacakges unavailable

@samoht
Copy link
Member

samoht commented Apr 7, 2023

Hey, could you tell us what the reason for this PR is?

Usually, the policy for opam-repository is to keep the old package version available as much as possible so that we can still run automated tests on those (for instance, on new compiler versions - which might not be very relevant there).

@killian-delarue
Copy link
Contributor Author

Hey, could you tell us what the reason for this PR is?

Usually, the policy for opam-repository is to keep the old package version available as much as possible so that we can still run automated tests on those (for instance, on new compiler versions - which might not be very relevant there).

These old packages are now useless as it can't be used on any octez networks. I suggest this PR in case it could help reduce the charge of these automated tests.

Do you think that they shouln't ?

Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

We’ve already discussed this before but agree as a general concept (packages unusable and unused)

Those packages should still pass the linter. You can check them all in one go using:

opam lint tezos-*/*/opam octez-*/*/opam | grep Errors

@avsm
Copy link
Member

avsm commented Apr 8, 2023

I'm not in agreement with making these packages uninstallable without good reason. If the issue is that automated tests shouldn't be run on these, then we should modify the test systems to have a better filter, and not apply a hammer of making entire swathes of packages uninstallable.

But once a package enters the opam repository, you don't know who the downstream users are, and it's often useful years down the road to rebuild older versions of packages. I'd like a broader discussion or justification before disabling hundreds of packages, please. Where was the previous discussion that @kit-ty-kate mentions?

@killian-delarue killian-delarue force-pushed the killian@octez@mark_old_packages_unavailable branch from 89a24df to 3a35975 Compare April 11, 2023 16:04
@emillon
Copy link
Contributor

emillon commented Apr 21, 2023

I opened #23697 to discuss about what should be done for this situation.

@mseri
Copy link
Member

mseri commented Jun 2, 2023

I was one of the advocate against removal, but I do think there is value in this PR marking the packages unavailable. The state of things is that most of them are not usable to interact with tezos but, nevertheless, we are still testing them all each times. People can still use them by allow availability in their own opam overlays, pinning to a specific version of opam (which should be fine if they are firm on older versions of packages) or ignoring the constraint manually.

The current core release is re-testing lots of tezos for all the versions, leading effectively to a large waste of resources (time and energy for sure): https://toxis.caelum.ci.dev/github/ocaml/opam-repository/commit/883b11d2cbddc106afbb20b05cfe1c27505e97d8
Were this be merged, we would have a rather smaller revdeps test matrix.

In fact, I start to think it may be worth start doing pruning for all packages at some point.

@emillon
Copy link
Contributor

emillon commented Jun 2, 2023

What do you think about the proposal in #23697?

@mseri
Copy link
Member

mseri commented Jun 2, 2023

I am just leaving a comment there. I think figuring out a proper policy will require more discussion though

@avsm avsm mentioned this pull request Jul 24, 2023
@avsm
Copy link
Member

avsm commented Jul 24, 2023

I agree that we need to test fewer revdeps, but the answer isn't necessarily to make perfectly installable packages uninstallable. I do think we should have hints in the opam metadata that is less strong than available to indicate that the package should be avoided in day-to-day CI (e.g. for revdeps), but included in bulk builds of the full repository. There is precedent for this in the use of avoid-version.

@tmcgilchrist
Copy link
Contributor

@avsm the other place to consider is when building documentation for ocaml.org. Perhaps this isn't the right place to discuss but the metadata you're talking about would benefit there too. We spend a good amount of time building older Tezos packages that might not be used, not longer relevant or actively discouraged due to bugs.

@mseri
Copy link
Member

mseri commented Sep 13, 2023

The CI just broke again mostly due to the thousands of revdeps jobs from tezos running in parallel PRs, this PR would have avoided a large part of those to be run. I do think that for the time being there is reason in marking these as unavailable.

Imho we can revert the change in the future once we have a working solution.

Ping @avsm

@mtelvers
Copy link
Contributor

mtelvers commented Mar 5, 2024

With fewer cluster resources at the moment, it would be great if this PR could be merged.

@mseri
Copy link
Member

mseri commented Mar 5, 2024

I am going on with this

@mseri mseri merged commit fc2da8d into ocaml:master Mar 5, 2024
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants