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 missing pin to libcxx-devel; fix libcxx run-export #315

Merged
merged 3 commits into from
Sep 4, 2024

Conversation

h-vetinari
Copy link
Member

Oversight in #311, see here

@conda-forge-webservices
Copy link

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/meta.yaml) and found it was in an excellent condition.

@h-vetinari h-vetinari added automerge Merge the PR when CI passes and removed automerge Merge the PR when CI passes labels Aug 30, 2024
@h-vetinari h-vetinari changed the title add missing pin to libcxx-devel add missing pin to libcxx-devel; fix libcxx run-export Aug 30, 2024
@h-vetinari h-vetinari removed the automerge Merge the PR when CI passes label Aug 30, 2024
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.

@isuruf, so it turns out that adding a version pin on libcxx-devel runs into a bunch of issues:

  • it conflicts with libcxx {{ cxx_compiler_version }} host pins (for packages depending on clangxx)
  • it would cause a bootstrap issue because we don't have libcxx X+1 when we first build clangdev X+1

The bootstrapping I think I solved with a174f3a, but for the first issue I'm not 100% sure.

I think we could loosen the libcxx pins to build here, but not 100% sure why those were there in the first place.

recipe/meta.yaml Outdated
Comment on lines 90 to 91
# Use the same requirements as the top-level requirements
- libcxx-devel {{ cxx_compiler_version }} # [osx]
- libcxx-devel # [osx]
Copy link
Member Author

Choose a reason for hiding this comment

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

I would have expected the "use same as top-level" to have something to do with having the same hash of the host environment and maybe not busting the cache to rebuild things unnecessarily; however, I'm not sure from 91ad7d6 resp. ac155e5 if that's actually the case? I've removed it now to try and see.

Copy link
Member Author

Choose a reason for hiding this comment

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

I should probably update the comment # Use the same requirements as the top-level requirements for these outputs, but since I don't know/understand the original reason, I can only remove them. If you have input on this, please let me know.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tentatively updating the comment now to be

# unpinned because we depend on clangxx here, which pins libcxx-devel

If we want to keep everything on the same libcxx version, we could go one step further and pin everything to {{ version }}, now that we're building libcxx before clangdev.

@isuruf
Copy link
Member

isuruf commented Aug 30, 2024

@h-vetinari
Copy link
Member Author

You need https://github.com/conda-forge/libcxx-feedstock/pull/190/files#r1737146437

Ah, thanks. OK, I was waiting for your feedback on that PR (and thought we'd want to fix the compilers first).

@h-vetinari
Copy link
Member Author

You need https://github.com/conda-forge/libcxx-feedstock/pull/190/files#r1737146437

I merged conda-forge/libcxx-feedstock#192, which is the backport that's relevant here, as our cxx_compiler_version is still on 16 in this PR (which is nice because it lets us do this without touching the current compiler default version or newest libcxx).

However, this is still not enough, because now that we want clangxx to run-depend on libcxx-devel {{ version }}, it can then not be installed anymore for outputs depending on clangxx, which want to use libcxx-devel {{ cxx_compiler_version }}.

That particular problem should be solvable by reinstating the commit (1ab6b2d) I had before for loosening the libcxx-devel constraint in outputs depending on clangxx, but I've realised that this still creates bootstrapping problems.

Whether we put the version pin in run_constrained: or in run: is immaterial, wanting to install clangxx during the build here means we're creating a cycle here with the libcxx feedstock, which depends on clangdev. The saving grace (perhaps?) is that it does so only on linux. IOW, we can build libcxx 19 on osx using older compilers (if we merge with broken CI on linux, or add a bootstrapping switch), and then build clangdev + the rest of the stack.

@h-vetinari
Copy link
Member Author

h-vetinari commented Aug 31, 2024

The saving grace (perhaps?) is that it does so only on linux. IOW, we can build libcxx 19 on osx using older compilers (if we merge with broken CI on linux, or add a bootstrapping switch), and then build clangdev + the rest of the stack.

Even better, I tried building libcxx without clangdev, and it builds correctly, no problems with the link check, and the content looks unchanged (no extra headers or libs).

h-vetinari added a commit to h-vetinari/libcxx-feedstock that referenced this pull request Aug 31, 2024
this does not seem necessary to build, and prevents us from letting `clangxx`
depend on `libcxx-devel` (because it would create a build cycle); see
conda-forge/clangdev-feedstock#315 (comment)
@h-vetinari
Copy link
Member Author

This and conda-forge/libcxx-feedstock#190 are now passing, and I believe should cover all bases, including the bootstrapping case. PTAL @isuruf

@h-vetinari
Copy link
Member Author

Gentle ping @isuruf

recipe/meta.yaml Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Member Author

If you have some input on the thread above, please let me know. :)

@h-vetinari
Copy link
Member Author

Alright putting this in for now, but happy to iterate further on the upcoming PRs for 19.1.0.rc4 & then 19.1.0

@h-vetinari h-vetinari merged commit 8119774 into conda-forge:main Sep 4, 2024
8 checks passed
@h-vetinari h-vetinari deleted the devel branch September 4, 2024 03:38
@h-vetinari h-vetinari mentioned this pull request Sep 4, 2024
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.

2 participants