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

Drop cos6 builds #6070

Merged
merged 1 commit into from
Jul 1, 2024
Merged

Drop cos6 builds #6070

merged 1 commit into from
Jul 1, 2024

Conversation

isuruf
Copy link
Member

@isuruf isuruf commented Jun 24, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@isuruf isuruf requested a review from a team as a code owner June 24, 2024 20:24
@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

@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.

Talk about an anti-climactic finish for the immense journey that cos6 has had in conda-forge... 😆

Closes conda-forge/conda-forge.github.io#1436

@mbargull
Copy link
Member

Do we have a task list in some issue for this already?
If not, we could add it to conda-forge/conda-forge.github.io#1436 .
Things apart from the pinning changes that come to mind:

  • adjust numbers of track_feature to sysroot_linux-64
    (somewhat obsolete due to {{ stdlib('c') }}; could be relevant for downstream users for a limited amount of time)
  • docs
  • (can be done later: remove linux-anvil-comp7 build files)

@h-vetinari
Copy link
Member

adjust numbers of track_feature to sysroot_linux-64 (somewhat obsolete due to {{ stdlib('c') }}; could be relevant for downstream users for a limited amount of time)

AFAIU the idea was to drop the sysroot repodata hacks completely.

@jakirkham
Copy link
Member

adjust numbers of track_feature to sysroot_linux-64 (somewhat obsolete due to {{ stdlib('c') }}; could be relevant for downstream users for a limited amount of time)

AFAIU the idea was to drop the sysroot repodata hacks completely.

cc @beckermr (in case you have thoughts 🙂)

@isuruf
Copy link
Member Author

isuruf commented Jun 24, 2024

AFAIU the idea was to drop the sysroot repodata hacks conda-forge/linux-sysroot-feedstock#63 (comment).

That would break non-conda-build users of our compilers since they would be getting 2.28 and would not be able to run executables made with our compilers if they have __glibc<=2.27

@mbargull
Copy link
Member

AFAIU the idea was to drop the sysroot repodata hacks completely.

That is a long term goal, yes.
Unless I'm mistaken, the prerequisite for that is {{ stdlib("c") }} usage -- which you've worked through for conda-forge.
For downstream users, we don't know when they'll use that.
Plus for non-conda-build compiles, people would also need to be explicit about the sysroot_linux-64 version they install.

@h-vetinari
Copy link
Member

h-vetinari commented Jun 24, 2024

This is Matt's comment I referenced:

@beckermr: Yeah to be clear, once we remove the hacks @h-vetinari is referring to, conda will install the latest sysroot when a compiler is installed. That will be w/e the latest is at the time, which right now is 2.28 IIRC. It won't mean we ship the compiler without the sysroot.


@isuruf: That would break non-conda-build users of our compilers

We could teach them to install sysroot_{{ platform }} 2.x, just like they've installed gcc_{{ platform }} already. Sidenote: ~90% of the ecosystem are on 2.28+ already.

@mbargull: Unless I'm mistaken, the prerequisite for that is {{ stdlib("c") }} usage -- which you've worked through for conda-forge.

Yeah, we're set for that in conda-forge now (modulo old recipes, but the linter warns on misuse at least).

For outside use, we should document what support we provide. Given that we have no stated support for this, we IMO shouldn't be shy about asking users that want to manually install our production compilers (rather than compilers, where we could easily depend on 2.17) to do the necessary changes w.r.t. stdlib, rather than keep the sysroot hacks indefinitely.

@beckermr
Copy link
Member

Even if we don't drop the hacks, I think we need to update them to make sure cos7 is highest priority.

@isuruf
Copy link
Member Author

isuruf commented Jun 24, 2024

We could teach them to install sysroot_{{ platform }} 2.x, just like they've installed gcc_{{ platform }} already.

Not really. The compilers are pulled in with r-base and other packages.

@h-vetinari
Copy link
Member

The compilers are pulled in with r-base and other packages.

Any reason we couldn't make those packages run-depend on {{ stdlib("c") }} (on linux), thus staying with 2.17?

@isuruf
Copy link
Member Author

isuruf commented Jun 24, 2024

Then you wouldn't be able to change c_stdlib_version to 2.28 in downstream recipes/users.

@beckermr
Copy link
Member

We could introduce the sysroot dependence only for the compilers metapackage maybe?

@h-vetinari
Copy link
Member

h-vetinari commented Jun 24, 2024

Then you wouldn't be able to change c_stdlib_version to 2.28 in downstream recipes/users.

It could work if we had wrappers for each sysroot version. Not saying this packaging complexity is desirable, but perhaps it's still better than indefinitely keeping the track_features:

outputs:
  - name: _r-base  # same as current r-base
      ...
  {% for glibc_ver in ["2.17", "2.28"] %}
  - name: r-base
    build:
      string: ...{{ glibc_ver }}...
    requirements:
      run:
        - {{ pin_subpackage("_r-base", exact=True) }}
        - sysroot_{{ target_platform }} {{ glibc_ver }}  # [linux]
  {% endfor %}

We could introduce the sysroot dependence only for the compilers metapackage maybe?

That makes sense in any case IMO, but if we don't do contortions along the lines of the above, it seems we cannot get rid of the need to weigh down the newer sysroot.

@beckermr
Copy link
Member

Ah sorry. We should be clear. There are hacks specifically for current_repodata.json. I think we should drop those. We still need the features etc unless always use stdlib which has other issues.

@beckermr
Copy link
Member

I definitely wanted to drop all of the hacks but Isuru is right. Just keeping build number and the track features is manageable.

@h-vetinari
Copy link
Member

How about we merge this PR and track other things in conda-forge/conda-forge.github.io#1436?

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.

One problem I noticed.

Comment on lines -17 to -18
- 2.12 # [linux64 and os.environ.get("DEFAULT_LINUX_VERSION", "cos6") == "cos6"]
- 2.17 # [linux64 and os.environ.get("DEFAULT_LINUX_VERSION", "cos6") == "cos7"]
Copy link
Member

Choose a reason for hiding this comment

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

Removing these lines will break the ability to set the linux version in smithy via os_version

https://github.com/conda-forge/conda-smithy/blob/9e4720fc497f45f45d7bd319a354c762992c530d/conda_smithy/configure_feedstock.py#L994

Maybe we keep them and set the default to cos7?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean you want to keep supporting cos6 in some feedstocks?

Copy link
Member

Choose a reason for hiding this comment

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

You mean you want to keep supporting cos6 in some feedstocks?

Right. there are two things. First yes, we no longer officially support cos6.

Second, are we deprecating os_version in smithy? If not, we need to keep the selectors here to account for smithy setting DEFAULT_LINUX_VERSION like this:

        # set the environment variable for OS version
        if platform == "linux":
            ver = forge_config["os_version"][f"{platform}_{arch}"]
            if ver:
                os.environ["DEFAULT_LINUX_VERSION"] = ver

We probably need to add them to add support for cos8 or higher, but in another PR maybe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Second, are we deprecating os_version in smithy? If not, we need to keep the selectors here to account for smithy setting DEFAULT_LINUX_VERSION like this:

We should. As I mentioned for cos8, we should change cdt_name to be conda and apply that for cos7 as well. Then we get rid of DEFAULT_LINUX_VERSION.

Copy link
Member

Choose a reason for hiding this comment

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

Great. Then lgtm on this and we can move the deprecation discussion to smithy.

@h-vetinari
Copy link
Member

Planning to merge this in ~24h if there are no further comments.

@jakirkham
Copy link
Member

jakirkham commented Jun 29, 2024

Let's do this during the week (not the weekend) so people are around to fix any issues that come up

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.

5 participants