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

Add first batch of alma8 CDTs #70

Merged
merged 19 commits into from
Sep 18, 2024
Merged

Add first batch of alma8 CDTs #70

merged 19 commits into from
Sep 18, 2024

Conversation

h-vetinari
Copy link
Member

This tackles the first batch of CDTs from #66, more specifically those currently marked ✅ here. Additionally, I've added:

  • The -devel packages from the dependencies we've added for pam; IMO it makes sense to always have libfoo and libfoo-devel for a given CDT, otherwise libfoo is going to be non-functional (unless as a dependency for another CDT)
  • mesa-libgbm because it was the only mesa-* package not to receive an ❌; I had asked if that was intentional, Isuru had answered in the affirmative (but only in a core call, not as a comment in the issue)
  • kmod-devel & mesa-libgbm-devel for the same reason; since we alread have kmod & mesa-libgbm

Doing this turned out to be a good exercise in the sense that the alma8 builds are separated across various subfolders that may depend on each other, which needed some updates to the logic.

Furthermore, I've switched away from skipped_cdts in favour of an allowlist-approach, as the impression I got from #66 was that people want to keep the list absolutely minimal, and it's easier to keep track of that using a positive list, rather than ensuring that we captured all exclusions correctly everywhere.

Finally, I repeatedly got permission errors (HTTP 403) when trying to urlopen https://vault.almalinux.org/8.9/BaseOS/{base_architecture}/os/repodata/repomd.xml, so I ended up switching to requests which doesn't have a problem there.

Given the large mechanical changes, it's probably easiest to review per commit.


Please see the repo readme for directions on how to make PRs on this repo.

Checklist:

  • if you have added a CDT, it appears in the cdt_slugs.yaml file and
    you have rerun the script python gen_cdt_recipes.py.
  • if you have changed the CDT generator code (rpm.py), you have bumped
    the build number in conda_build_config.yaml
    and have remade all of the
    recipes via running python gen_cdt_recipes.py --force
  • if you have added a custom CDT recipe, you have added the name of the CDT
    with custom: true in the cdt_slugs.yaml file.
  • all CDT recipes have build number set by {{ cdt_build_number }} for
    old-style/legacy CDTs or {{ cdt_build_number|int + 1000 }} for new-style CDTs
  • if you see a warning about a CDT not having a license, you have added the
    license_file key in the cdt_slugs.yaml file with the path to the appropriate
    license in licenses/

NOTE: If you make any changes to cd_slugs.yaml, you need to reun the generator code
via python gen_cdt_recipes.py.

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Let's put the commits of the new CDT recipes into another PR. I cannot review the new logic commit by commit.

@h-vetinari
Copy link
Member Author

Let's put the commits of the new CDT recipes into another PR.

It's hard to validate the logic without actually rendering & building things... But if you prefer, I'll revert this at the end once it's green (or rebase it out) for ease of review.

I cannot review the new logic commit by commit.

Aside from the one rerender commit, the diffs are tiny, and GH makes it easy to click through (prev/next)...?

@beckermr
Copy link
Member

I'm not currently figuring out how to do it for the commits but ok. I'll do my best.

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

The yaml file had support for other things that had been important:

  1. excluding certain deps (e.g., kernel-headers)
  2. putting in custom logic in the build scripts for certain CDTs

When you added alma8 with the allow list, you didn't implement those features for alma8. How do you know we won't need them? Why not stick to the yaml so we can keep them?

@beckermr
Copy link
Member

One concrete way to do this would be to have a separate cdt slugs file for alma8 and cos7. Then each one is an allow list on its own and we can keep the extra hooks if we need them.

@h-vetinari
Copy link
Member Author

The yaml file had support for other things that had been important:

None of the logic except skipped_cdts is changed. Removing/replacing deps & custom logic is still possible

Why not stick to the yaml so we can keep them?

I haven't changed away from the yaml.

@beckermr
Copy link
Member

So I have to list an alma8 CDT in both the yaml AND the allow list in the python file?

@beckermr
Copy link
Member

Maybe the better question is, can you show me how to add a skipped dependency for an alma8 CDT?

@h-vetinari
Copy link
Member Author

h-vetinari commented Sep 17, 2024

One concrete way to do this would be to have a separate cdt slugs file for alma8 and cos7.

So, with the caveat that all of this is still a draft, I'd prefer not to split the cdt slugs, because for all the ones we're interested (so far), the config is exactly the same as for cos7, so duplicating them would be kinda gratuitous.

FWIW, I had started with applying the skipped_cdts approach, and it just ends up with

  skipped_cdts:
    - alma8-x86_64
    - alma8-aarch64
    - alma8-ppc64le

for almost all slugs.

@h-vetinari
Copy link
Member Author

So I have to list an alma8 CDT in both the yaml AND the allow list in the python file?

If it's an entirely new CDT then yes. If it's a CDT that's already on cos7, then only the allowlist. Again, please consider this a suggestion (it seemed more natural given how much people want to constrain CDT creation), not as something set in stone.

cdt_slugs.yaml Outdated Show resolved Hide resolved
@beckermr
Copy link
Member

OK thanks for answering my questions @h-vetinari!

Maybe we can refactor the cdt_slugs.yaml file to look like this:

allowed_cdts:
  alma8:
    - foo
    - bar
    - blah
  cos7:
    - foo
    - bar
    - blah
    - glarg
cdt_build_defs:
  # any CDT not listed here is equivalent to the default
  #
  #   <cdt name>:
  #     custom: false
  #
  foo:
    license_file: licenses/alsa-lib-license
  bar:
    dep_remove:
      - foobarg

This way all of the configuration is still in one spot and we only have to list things that are special/different.

@h-vetinari
Copy link
Member Author

Maybe we can refactor the cdt_slugs.yaml file to look like this:

We can. Though it'll bust the indentation of everything in that file to introduce a new level (cdt_build_defs). 🤷

@beckermr
Copy link
Member

Yeah I am ok with the indentation change.

@h-vetinari
Copy link
Member Author

I'm not currently figuring out how to do it for the commits but ok. I'll do my best.

FWIW, there's a commit tab on top (between "Conversation" and "Checks"); if you click on the first commit therein, you'll get a prev/next navigation menu in the upper RH corner. Aside from the rerender commits (where you can just click next without review), the others are very small and should be easy to digest.

Yeah I am ok with the indentation change.

I might not finish this today, but it shouldn't be hard conceptually; for now I made the change that Isuru asked for, and it's getting late here.

@beckermr
Copy link
Member

No rush on this and thanks for the dev work @h-vetinari!

@h-vetinari h-vetinari force-pushed the alma branch 4 times, most recently from 7d23d2c to 5014799 Compare September 17, 2024 23:27
Copy link
Member Author

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Another infrastructure question

cdt_slugs.yaml Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Member Author

This way all of the configuration is still in one spot and we only have to list things that are special/different.

From a quick check, only 30 CDTs out of 180 would actually fall under

  # any CDT not listed here is equivalent to the default
  #
  #   <cdt name>:
  #     custom: false

It's a bit hard to argue that 83% are the special case IMO 😅

The vast majority of those only specifies the license_file, but unless we have a way to revamp the license handling too, I'm not sure this would be an improvement. It'd just be confusing look for a non-existent build definition of a CDT that you're interested in (e.g. often libfoo-devel needs a license override, but libfoo doesn't), only to realise you've fallen into the fallback case.

@beckermr
Copy link
Member

We can list them all then, that's fine by me. What doesn't work is having to add a CDT name in two places, once in a yaml file and once in the python code, in order to add a new CDT. That is confusing for sure.

ignore whitespace in git to see that this changes nothing, e.g.
`git show -b <hash_of_this_commit>`, or add `?w=1` to the GH url
@h-vetinari
Copy link
Member Author

@beckermr, I implemented your idea (minus the default fallback, as discussed), and a run of python gen_cdt_recipes.py --force yields no further changes (as desired). I added some basic validation (tested to work locally) to fail early in case of missing build_defs, or warn in case of superfluous ones.

Feel free to play with this branch according to your usual workflows, happy to fix. :)

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@isuruf
Copy link
Member

isuruf commented Sep 18, 2024

Why is there a java CDT? Is it a transitive dependency?

@h-vetinari
Copy link
Member Author

Why is there a java CDT? Is it a transitive dependency?

I didn't touch those, they're pre-existing (also present in the list in #66 since last Nov.). I looked for current users and even checked now if they were used in any of the non-java packages we dropped in 779da0b, but can't find any users in the CDTs themselves.

Looking around conda-forge, it's used, but only in r-base. Once you remove that, we can consider dropping them here.

@isuruf
Copy link
Member

isuruf commented Sep 18, 2024

Ah, it's just a whitespace change.

@h-vetinari h-vetinari merged commit d020455 into conda-forge:main Sep 18, 2024
7 checks passed
@h-vetinari h-vetinari deleted the alma branch September 18, 2024 18:18
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks all for getting this going! 🙏

Had a question below about naming/versioning

Comment on lines +293 to +294
Build all CDT recipes for a given DIST_ARCH_SLUG (e.g. conda-x86_64
for post-CentOS CDTs, or cos6-x86_64, cos7-aarch64, etc. historically)
Copy link
Member

Choose a reason for hiding this comment

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

Noticing that the new slugs don't have a version. Did we want to bake one in like we did with cos6 & cos7? For example conda-2_28-<arch>

Copy link
Member

@isuruf isuruf Oct 15, 2024

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants