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

capture build string variations in hdf5 1.14.3 patch #614

Merged
merged 1 commit into from
Dec 9, 2023

Conversation

minrk
Copy link
Member

@minrk minrk commented Dec 8, 2023

Update rule in #611 to capture dependencies with build string pinning, which would be anything that build against hdf5 with mpi, e.g. petsc.

Checklist

  • Used a static YAML file for the patch if possible (instructions).
  • Only wrote code directly into generate_patch_json.py if absolutely necessary.
  • Ran pre-commit run -a and ensured all files pass the linting checks.
  • Ran python show_diff.py and posted the output as part of the PR.
  • Modifications won't affect packages built in the future.

showdiff output

@minrk minrk requested a review from a team as a code owner December 8, 2023 20:29
@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

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 Min! 🙏

Had one question above

has_depends: hdf5 >=1.14.2,<1.14.3.0a0
timestamp_lt: 1701868616000
has_depends: hdf5 >=1.14.2,<1.14.3.0a0?( *)
timestamp_lt: 1702066879000
Copy link
Member

Choose a reason for hiding this comment

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

Could you please share a bit about the timestamp change? It appears the new timestamp is more recent than the previous one

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like the timestamp should be removed for this patch, we are claiming ABI compatibility, so it should be fine to remove outright.

The problem is that packages may still try to build with the old pin.

Copy link
Member

@jakirkham jakirkham Dec 9, 2023

Choose a reason for hiding this comment

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

Wonder if we should just rebuild the hdf5 packages to include a better run_exports line

While in theory dropping the timestamp condition would mean anything would be patched. Really it would mean we patch the repodata based on whenever this feedstock last built a package with a new patch. Though when that happens is at some arbitrary time (in contrast to being truly unbounded)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is really out of scope for now.

We should really just answer this question in my mind:
conda-forge/conda-forge-pinning-feedstock#3368

For now this PR looks like a step in the right direction. the timestamp still exists so itisn't so wide reaching, but there is still a risk of systems being built with 1.14.2 instead of 1.14.3 that would carry the 'x.x.x' limitations

@hmaarrfk hmaarrfk merged commit bc26085 into conda-forge:main Dec 9, 2023
4 checks passed
@hmaarrfk
Copy link
Contributor

hmaarrfk commented Dec 9, 2023

Thank you.

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

Successfully merging this pull request may close these issues.

3 participants