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

Target main label for 8.1.2 builds #135

Merged
merged 5 commits into from
Jul 16, 2024
Merged

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Jul 16, 2024

#133 (comment)

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.

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

I do have some suggestions for making it better though...

For recipe:

  • This recipe is using a compiler, which now requires adding a build dependence on {{ stdlib("c") }} as well. Note that this rule applies to each output of the recipe using a compiler. For further details, please see META: {{ stdlib("c") }} migration conda-forge.github.io#2102.
  • You're setting a requirement on sysroot_linux- directly; this should now be done by adding a build dependence on {{ stdlib("c") }}, and overriding c_stdlib_version in recipe/conda_build_config.yaml for the respective platform as necessary. For further details, please see META: {{ stdlib("c") }} migration conda-forge.github.io#2102.
  • You are setting MACOSX_SDK_VERSION below c_stdlib_version, in conda_build_config.yaml which is not possible! Please ensure MACOSX_SDK_VERSION is at least c_stdlib_version (you can leave it out if it is equal).
    If you are not setting c_stdlib_version yourself, this means you are requesting a version below the current global baseline in conda-forge (10.13). If this is the intention, you also need to override c_stdlib_version and MACOSX_DEPLOYMENT_TARGET locally.

@mattwthompson
Copy link
Member Author

@conda-forge-admin, please rerender

conda-forge-webservices[bot] and others added 2 commits July 16, 2024 15:29
@peastman
Copy link
Contributor

@conda-forge-admin, please rerender

@mattwthompson
Copy link
Member Author

Stuff starting to come in green now

@mattwthompson mattwthompson marked this pull request as ready for review July 16, 2024 16:52
Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

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

As I said at #133 (comment), this needs changes.

Please do not merge this

@isuruf
Copy link
Member

isuruf commented Jul 16, 2024

If you don't want to do changes, please revert the rerender. If you do the rerender, you need to fix the lints.

@isuruf
Copy link
Member

isuruf commented Jul 16, 2024

From the rerender, I mean the rerender done at previous PR

@peastman
Copy link
Contributor

The lint comment says,

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.

I do have some suggestions for making it better though...

This is a patch release. We never make any changes to a patch release, no matter how tiny, except what is absolutely required to fix bugs. Every change, no matter how tiny, has the potential to cause problems. "Suggestions for making it better" do not qualify for inclusion in a patch release. They have to wait for the next major version.

@isuruf
Copy link
Member

isuruf commented Jul 16, 2024

Then please revert the rerender that changed MACOSX_DEPLOYMENT_TARGET.

@peastman
Copy link
Contributor

I don't think I understand what you mean. How do we revert a rerender? Rendering is required before every build, and we can't control what changes it makes.

@isuruf
Copy link
Member

isuruf commented Jul 16, 2024

Revert all the changes in .ci_support folder from the last correct build.

@peastman
Copy link
Contributor

Why? I'm confused about what problem you're trying to solve. The very first step required in every PR is to rerender. If you don't do that, it usually won't build. Can you explain what the issue is?

@peastman
Copy link
Contributor

All builds succeeded. So whatever the problem is, it clearly isn't something that prevents building successfully.

@isuruf
Copy link
Member

isuruf commented Jul 16, 2024

Changes in .ci_support change the way builds are done and when you say "We never make any changes to a patch release, no matter how tiny, except what is absolutely required to fix bugs", but do a rerender, you are making big changes. For example 8.1.1 works on macOS 10.9 and if you merge this PR, 8.1.2 would not work on macOS 10.9 and fail when installed. Adding stdlib would make sure this package does not get installed on macOS 10.9.

@peastman
Copy link
Contributor

If we don't make the changes to .ci_support, it won't even build.

@peastman
Copy link
Contributor

I'm going to go ahead and merge. We really need to get this out, and the suggestion above is not a realistic one. It would simply break the build.

@peastman peastman merged commit 7541405 into conda-forge:main Jul 16, 2024
66 checks passed
@isuruf
Copy link
Member

isuruf commented Jul 16, 2024

That's not really how open source communities work. I've cancelled the builds for now. Please don't merge without addressing comments.

@isuruf
Copy link
Member

isuruf commented Jul 16, 2024

@conda-forge/core, can we get a resolution here please?

@peastman
Copy link
Contributor

@conda-forge/core I am raising a complaint against @isuruf under the code of conduct. This is blatant interference with how we develop a product that he is not a developer on. His actions here amount to pure trolling.

Isuru, useful suggestions are always welcome, but it is not your decision, and you do not have a veto on when we get to perform builds. The suggestion you made was a patently unrealistic one that would simply break the build, as you knew perfectly well when you suggested it. It also would have completely violated conda-forge policy. Blocking us from creating builds is outside any acceptable behavior.

@jaimergp
Copy link
Member

Hi @peastman, as per my understanding, Isuru has stopped the builds to prevent packages from being uploaded with wrong metadata. The stdlib changes alone will bump your min OSX requirements, which contradicts the intent of "having the same conditions as in 8.1.1`.

If you really want to build something under the same circumstances as 8.1.1 (which is not really supported, but it might work), I think the the best attempt would be to take the exact same commit in this feedstock that built 8.1.1 and update meta.yaml to point to 8.1.2 (in addition to the metadata constraints). By rerendering, you are applying updates in the infra, pinnings and what not, so you are changing a lot of the context for the build. Right now the feedstock is in a weird intermediate state that needs some fixes for perfect compliance (which is what Isuru has been suggesting since the previous PR).

As a reminder, conda-forge is not a free release service users can customize to their exact needs. It's a community effort where each component strives to be a team player and tries to compromise the needs of the organization with that of the project. There should be no expectations of availability or response times, either.

I hope we can all reach a compromise here.

PS: It's quite late here, so please apologize if I don't reply to further messages til tomorrow morning.

@peastman
Copy link
Contributor

Hi @jaimerpg. I understand exactly what the effects of merging are. Understanding that, I chose to merge this PR. He blocked it from building not to help us, but because we didn't make his totally unrealistic change to revert the conda-smithy render to a version from several months ago that would no longer build (as he knew perfectly well).

Everything in this PR is 100% compliant with current policies. The linter reported it as being "in an excellent condition."

@h-vetinari
Copy link
Member

The linter reported it as being "in an excellent condition."

This is boilerplate and we don't change this text if we find what we consider "hints" (but really: warnings), which the linter comment clearly contains.

Isuru was well within his rights to request the changes he did, and while you are the maintainer on this feedstock, conda-forge/core are the maintainers of the entire conda-forge infrastructure. The overall infrastructure changed (see mainly here), and your feedstock must follow, regardless of whether you only consider this a patch release.

For example, the underlying glibc version pulled in by the sysroot (i.e. the baseline version the package ends up being compiled against) changed from 2.12 to 2.17, completely independently of any changes to this feedstock. If you merge a PR after that without adding {{ stdlib("c") }}, you are creating artefacts with broken metadata (i.e. they will appear installable on glibc <2.17, but break if anyone actually does that). You might not know about this, but Isuru does. I agree with him cancelling your builds after you brushed off his review. Please open a new PR that performs the requested changes.

@beckermr
Copy link
Member

Hi everyone.

I want to apologize for any confusion we caused due to the rollout of the new stdlib handing. We did the best we could, but as usual, we may have missed things. Specifically, we likely should have moved the hints to lints for the stdlib-related items given that they are indeed required. I have opened a PR on conda-smithy with this change and discussion of this change should be directed there. Further, documentation for the stdlib handling and its edge cases is not yet fully complete (see, e.g., conda-forge/conda-forge.github.io#2206). The news item (https://conda-forge.org/news/2024/03/24/stdlib-migration/) from March 2024 is the best documentation that I know of right now. We of course need to work on updating the docs.

It is useful in these cases to look back at the history of conda-forge (which @isuruf knows better than me and parts of which are in our docs) and its reason for existing in the first place. In the before times, different conda channels on anaconda.org were broadly incompatible. So even if you managed to build your own packages for conda and upload them, you'd run into sometimes-subtle errors related to compatibility. Further, building the conda packages themselves was a somewhat bespoke process (and still is in many ways). conda-forge was created to help address these broad compatibility issues and to help folks build their own packages. Specifically, per @h-vetinari and @jaimergp's comments, part of the mandate of the core team is to ensure broad compatibility as this is one of conda-forge's core missions.

I want to reiterate that @isuruf is 100% correct on the technical issues with the recipe, its metadata, what kinds of changes were merged due to rerendering, etc. The core team has and continues to reserve the right to request changes to recipes for the greater good of the ecosystem. Please see our governance. We do our best and don't take potentially destructive actions lightly.

Finally, I do want to thank everyone here for their contributions to conda-forge. We don't do this enough and it is important to recognize the hard work all of us have put into the ecosystem.

@pkgw
Copy link

pkgw commented Jul 17, 2024

Thanks @beckermr for putting in the work to try to make the linter more helpful here. I agree that the stdlib hints should be upgraded to warnings, especially now that we have some experience with the stdlib migration and (as far as I know) things have more or less stabilized with how it's proceeding.

More broadly, I just want to add that I've always believed that conda-forge's bread and butter — taking a bunch of independently-developed pieces of software and unifying them into a coherent ecosystem of interoperable packages — is just as much of a challenging and important undertaking as the development of the underlying codebases. At times it can be substantially more challenging because there are deeper social dimensions to the work, in addition to technical ones. Because the very heart of conda-forge is this never-ending work to bring together a huge range of disparate projects under one roof, it's extremely important to listen to what others are saying even — especially — when we don't agree.

I'll also repeat @beckermr thanks to everyone for all the work they do every day to create this invaluable resource.

@peastman
Copy link
Contributor

Thanks, I really appreciate the clarification. To be clear, what you are talking about is making the changes described by the linter, not reverting all the changes made by conda-smithy to a version from several months ago as Isuru said to do. Is that correct?

How urgent are those changes? The only intent of this PR is to fix an incorrect tag on the previous build, nothing more.

@beckermr
Copy link
Member

Thanks, I really appreciate the clarification. To be clear, what you are talking about is making the changes described by the linter, not reverting all the changes made by conda-smithy to a version from several months ago as Isuru said to do. Is that correct?

I'm happy to help clarify things.

I made changes to the linter to explicitly require the addition of stdlib jinja2 etc. to the recipe as @isuruf requested originally.

How urgent are those changes? The only intent of this PR is to fix an incorrect tag on the previous build, nothing more.

Those changes are urgent in the sense that if you want to produce builds with correct metadata and upload them to conda-forge, then they need to be made. When you want to do that is up to you.

@peastman
Copy link
Contributor

Got it thanks.

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.

7 participants