-
-
Notifications
You must be signed in to change notification settings - Fork 279
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
update infrastructure.rst/compilers #1946
Conversation
src/maintainer/infrastructure.rst
Outdated
compilers, we have historically maintained them according to the following (non-binding) | ||
principles. | ||
|
||
* The authoritative source of the current compilers and versions for various languages | ||
and platforms is the `conda_build_config.yaml <https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/master/recipe/conda_build_config.yaml>`_ | ||
in the `conda-forge/conda-forge-pinning-feedstock <https://github.com/conda-forge/conda-forge-pinning-feedstock>`_ | ||
as described in :ref:`globally_pinned_packages`. | ||
* We provide no support of any kind in terms of the long-term stability of these pinnings. | ||
* We upgrade them in an ad-hoc manner on a periodic basis as we have the time and energy to do so. | ||
Note that because of the way we enforce runtime constraints, these compiler upgrades will not break | ||
existing packages. However, if you are using the compilers outside of ``conda``, then you may find issues. | ||
* We generally provide notice in the form of an announcement when a compiler is going to be upgraded. | ||
Note that these changes take a bit of time to complete, so you will generally have time | ||
to prepare should you need to. | ||
* Some of the criteria we think about when considering a compiler migration include | ||
1) the degree of disruption to the ecosystem, 2) the amount of work for the ``core`` team, | ||
and 3) the amount of time it will cost our (volunteer) feedstock maintainers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of the language here was agreed upon by the core team and should stay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I've put them back in with the exception of the last two, which aren't relevant anymore, since we don't have to migrate to change the compiler version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I'd argue that even the middle two points are not relevant anymore either (and the first one I had kept in the running text), again because of the way how there's no more monolithic stack.
Awesome. Thank you Axel! 🙏 |
src/maintainer/infrastructure.rst
Outdated
* We generally provide notice in the form of an announcement when a compiler is going to be upgraded. | ||
Note that these changes take a bit of time to complete, so you will generally have time | ||
to prepare should you need to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you asking why I removed this?
Because AFAIK the last gcc & clang updates didn't come with an announcement, and that in turn because it's no as big of a deal anymore as changing to comp7 (which is noted further below in that document as "our current compiler stack") was once upon a time.
Nowadays, gcc & clang bumps just percolate through the ecosystem at the speed with which feedstocks get rerendered, without compatibility constraints (well, aside from small issues here and there, but not large-scale ABI incompatibilties).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not about ABI compatibilites and if you look at the git history it is not about gcc 4->7 migration.
aside from small issues here and there, but not large-scale ABI incompatibilties
Exactly why announcements are there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly why announcements are there
If someone had asked me for an announcement for conda-forge/conda-forge-pinning-feedstock#4215, I would have done it, no problem. However, neither were clang 13, clang 14 or GCC 11 announced; also it's the first time during all those bumps (from what I saw) that someone notices a breakage at all.
This is not about ABI compatibilites and if you look at the git history it is not about gcc 4->7 migration.
OK, I'll readd the announcement bit. However the "Note that these changes take a bit of time to complete, so you will generally have time to prepare should you need to." is not true anymore AFAICT.
Some of this old text refers to cases where have had to do full ABI migrations. It is also still relevant to some backwards incompatible changes we make on the windows stack without doing ABI migrations. I know this wasn't clear, but that was the context for many of these comments. |
We will eventually have to an ABI migration like this in the future. |
Which ones? We did VS2017 -> VS2019 without a migration.
Short of the C/C++ standard explicitly breaking ABI (or Microsoft following through on its vNext proposal), what reasons for a full ABI migration do you foresee? It's fine to keep some historical context about full ABI migrations, but it's not representative of the status quo of the last ~3 years, and neither IMO for the foreseeable future. |
We had one in Fortran last and one of the warnings above was explicitly for the windows change which iirc should have had a migration formally. |
Fortran is a small subset of our feedstocks, so it's far from a conda-forge-wide migration like we'd need if the C/C++ ABI changes. (for reference, conda-forge/conda-forge-pinning-feedstock#1359 would have been ~300 feedstocks if it's across all platforms; windows-only it's much smaller still). Everyone from the C/C++ standards to the compiler & OS vendors, etc. is careful-verging-on-paranoid about ABI changes. It seems to me that conda-forge has over time managed to find a setup that doesn't expose itself to additional ABI breaks. So I honestly don't see where ABI breaks requiring a full migration would come from in our current setup.
I don't know what you're referring to here. |
This documentation enumerates our effective policies for ABI breaks of this kind. They've happened in the past and I don't think it's unreasonable for one to expect that they may happen sometime in the future. When that happens, we'll use this documentation to recall what we've done in the past in order to inform our actions. Conda-forge's lifetime and relevant timescales are intended to be measured in decades. It could be five years before we need these policies again but nevertheless we should preserve them. |
Sure, only currently it's clothed as "compiler infrastructure" and not "ABI break" (because they used to be synonymous), and it takes an old hand to know this distinction.
Fair enough, but it needs to be distinguished from regular (non-ABI-breaking) compiler version bumps. |
There is another dimension here which is that some changes we make in conda forge can effect external users of the compilers. This is @isuruf's point iiuic. |
It might mean that they cannot use our newest artefacts anymore (or have to upgrade something themselves). That was the case for VS2019 (which we announced), and it will be the same for bumping the glibc or MacOS SDK lower bound eventually. I'm not in any way opposed to announcing things, we can even make it mandatory for compiler version bumps too. The key point is that since the comp7 days (when this section of the docs was written), there's now a distinction between a version bump and a full ABI migration, and these two need to have different policies. |
I made a smaller preparatory commit that keeps the existing policies in the context of ABI breaking migrations, and then added the existing setup separately in another commit. Hope this is more along the lines of what you had in mind. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great @h-vetinari. Thank you so much! I made a few minor changes to the text, but otherwise LGTM!
Co-authored-by: Matthew R. Becker <[email protected]>
* We upgrade them in an ad-hoc manner on a periodic basis as we have the time and energy to do so. | ||
Note that because of the way we enforce runtime constraints, these compiler upgrades will not break | ||
existing packages. However, if you are using the compilers outside of ``conda``, then you may find issues. | ||
* We generally provide notice in the form of an announcement when a compiler is going to be upgraded. | ||
* We usually provide notice in the form of an announcement when an ABI-incompatible compiler change is going to happen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please keep this as it was? It was agreed upon in a core meeting and I don't want to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @beckermr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes will do. My apologies. Not right this minute though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. #1947
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. #1947
We're not in such a rush IMO... I didn't press merge here, I'd be happy to discuss this some more. Aside from a typo ("conpiler"), #1947 is IMO not correct, because that section now explicitly tries to draw a distinction between ABI-breaking and ABI-preserving compiler upgrades, and you made that distinction less clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix the typo ofc but I'm not gonna split hairs on the language here. Isuru is correct that this language was agreed upon in a core meeting and so should remain unchanged. I mentioned this in the previous pr as well at some point iirc.
Please accept my apologies for merging too early. The pr looked good to go and so I went ahead.
I'm happy to back out the prs or we can continue debate in a new one.
I really appreciate the hard work from you @h-vetinari on this.
One thing I realized is that we don't separate out what is an informal policy from core and what is more general documentation. That is making this process more difficult than it has to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this language was agreed upon in a core meeting and so should remain unchanged
I disagree with the tenet that something that has been (core-)agreed once is forever unchangeable, especially if circumstances change.
You can back out my PR (or not 🤷); my interest here is not to offend but to provide useful documentation, in particular the distinction between ABI-breaking and not ABI-breaking compiler upgrades1. If that needs a core vote, then that's fine by me, but I wrote this text specifically with that separation in mind. If people don't want that separation, just revert.
One thing I realized is that we don't separate out what is an informal policy from core and what is more general documentation. That is making this process more difficult than it has to be.
👍
Footnotes
That's not true at all. We've been upgrading clang compilers way before that commit. |
Can we agree that an ABI-breaking compiler change is harder to pull off than an ABI-preserving one? If so, it would be natural to have policies that reflect the different level of effort required (which is what I tried to do here). The fact that previously, linux had breaking migrations and clang didn't (even in the comp7 days) is immaterial to whether that distinction makes sense. |
As discussed in conda-forge/conda-forge-pinning-feedstock#4215.
This is only my understanding, consider it a draft proposal.
I removed a lot of the language around the lack of support and how to upgrade, because we're now long past the
comp7
days where changing the compilers needed a conda-forge wide migration.I'm sure this can still be improved in a lot of ways, but I'd say it's already an improvement over the (very stale) status quo.
PTAL @isuruf @xhochy @jakirkham @beckermr
CC @conda-forge/core