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

poetry v1.8.3 #99

Merged

Conversation

regro-cf-autotick-bot
Copy link
Contributor

@regro-cf-autotick-bot regro-cf-autotick-bot commented May 8, 2024

It is very likely that the current package version for this feedstock is out of date.

Checklist before merging this PR:

  • Dependencies have been updated if changed: see upstream
  • Tests have passed
  • Updated license if changed and license_file is packaged

Information about this PR:

  1. Feel free to push to the bot's branch to update this PR if needed.
  2. The bot will almost always only open one PR per version.
  3. The bot will stop issuing PRs if more than 3 version bump PRs generated by the bot are open. If you don't want to package a particular version please close the PR.
  4. If you want these PRs to be merged automatically, make an issue with @conda-forge-admin,please add bot automerge in the title and merge the resulting PR. This command will add our bot automerge feature to your feedstock.
  5. If this PR was opened in error or needs to be updated please add the bot-rerun label to this PR. The bot will close this PR and schedule another one. If you do not have permissions to add this label, you can use the phrase @conda-forge-admin, please rerun bot in a PR comment to have the conda-forge-admin add it for you.

Dependency Analysis

Please note that this analysis is highly experimental. The aim here is to make maintenance easier by inspecting the package's dependencies. Importantly this analysis does not support optional dependencies, please double check those before making changes. If you do not want hinting of this kind ever please add bot: inspection: disabled to your conda-forge.yml. If you encounter issues with this feature please ping the bot team conda-forge/bot.

Analysis by grayskull shows a discrepancy between it and the the package's stated requirements in the meta.yaml.

Packages found by grayskull but not in the meta.yaml:

  • python >=3.8.0,<4.0.0
  • pkginfo >=1.10.0,<2.0.0

Packages found in the meta.yaml but not found by grayskull:

  • python >=3.8,<4.0
  • __win
  • __osx
  • __linux
  • cachecontrol-with-filecache
  • pkginfo >=1.9.4,<2.0.0

This PR was created by the regro-cf-autotick-bot. The regro-cf-autotick-bot is a service to automatically track the dependency graph, migrate packages, and propose package version updates for conda-forge. Feel free to drop us a line if there are any issues! This PR was generated by https://github.com/regro/cf-scripts/actions/runs/9009011030 - please use this URL for debugging.

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

  • You're setting a constraint on the __osx virtual package 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.

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

@xylar
Copy link
Contributor

xylar commented May 9, 2024

I think the linter was complaining when it shouldn't but it's easy to hide by switching to __unix...

@xylar
Copy link
Contributor

xylar commented May 9, 2024

@conda-forge-admin, please rerender

@xylar
Copy link
Contributor

xylar commented May 9, 2024

Just making sure conda-smithy won't get confused by the change I made.

Copy link
Contributor

github-actions bot commented May 9, 2024

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/poetry-feedstock/actions/runs/9010769264.

@xylar xylar added the automerge Merge the PR when CI passes label May 9, 2024
@github-actions github-actions bot merged commit 599a633 into conda-forge:main May 9, 2024
5 checks passed
Copy link
Contributor

github-actions bot commented May 9, 2024

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

@regro-cf-autotick-bot regro-cf-autotick-bot deleted the 1.8.3_h779967 branch May 9, 2024 01:22
@maresb
Copy link
Contributor

maresb commented May 9, 2024

@xylar, something's off with my understanding of this trick of requiring the virtual packages. My evidently-incorrect understanding is that the # [osx] selector is equivalent to "if __osx is in the dependencies". Checking the metadata of the packages this produced, both linux and osx depend on __unix, and xattr is a dependency of osx and not linux. So everything is working correctly, even though I expected it shouldn't.

Do you understand what is going on? How can the selectors distinguish between osx and linux when we require only __unix? I would hypothesize that # [osx] means "if __osx or __unix is in the dependencies", but that can't be correct since the selector would be true on Linux as well. So if the virtual package requirement isn't defining the selectors, then what does it accomplish?

@xylar
Copy link
Contributor

xylar commented May 9, 2024

@maresb, I was just following the knowledge base. But my understanding is that the __unix metapackage will just have the __osx metapackage in it for osx and the __linux one for linux. So it's exactly equivalent to what we had before but one line fewer and it makes the overzealous linter happy.

@BastianZim
Copy link
Member

The virtual packages are created here in conda (I think, has been a while since I checked that) and are just true for whatever platform you specify them.

The reason - xattr >=1.0.0,<2.0.0 # [osx] works is because we are building on all three platforms:

noarch_platforms:
- linux_64
- osx_64
- win_64

That means that only the OSX build will resolve to true and include xattr. The linux build is built on Linux and will not include it. That is separate from the virtual packages in the meta.yaml.

A normal noarch package will build only on Linux so there it wouldn't work.

(At least that's how I understood your question?)

@maresb
Copy link
Contributor

maresb commented May 9, 2024

Taking a fresh look at this, it seems to me like __unix is not equivalent and actually problematic.

I believe we're generating two interchangeable __unix builds, one with xattr and one without. Consequently, when the solver looks for available packages on osx, it also sees the Linux build as a viable candidate.

Unfortunately now I can successfully solve for an environment which should be inconsistent (via conda-lock since I don't have a mac)

channels:
- conda-forge
dependencies:
- poetry ==1.8.3
- xattr <1
platforms:
- osx-64

And indeed it's picking the Linux build without xattr.

To avoid problems, perhaps we should revert the __unix commit, make a build 1, and then mark build 0 as broken?

@BastianZim
Copy link
Member

Hmm I haven't tested it yet but that does make sense, true.

I have a Mac, anything specific I should test?

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

Appreciate you pointing out this issue to us in comment: conda-forge/conda-forge.github.io#2102 (comment)

Would suggest ignoring the linter in this case. We probably need to fix the linter somehow. If you have time to file a conda-smithy bug referencing this issue, that would be very helpful 🙂

@@ -59,9 +59,8 @@ requirements:
- xattr >=1.0.0,<2.0.0 # [osx]
Copy link
Member

Choose a reason for hiding this comment

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

Think the challenge with using __unix here is this dependency will either install on macOS and Linux or it will be ignored altogether

Instead would suggest ignoring the linter and sticking with what you had before

Comment on lines +62 to +63
- __unix # [unix]
- __win # [win]
Copy link
Member

Choose a reason for hiding this comment

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

IOW keeping the Linux and macOS distinction here

Suggested change
- __unix # [unix]
- __win # [win]
- __linux # [linux]
- __osx # [osx]
- __win # [win]

@maresb
Copy link
Contributor

maresb commented May 9, 2024

@BastianZim you could try conda install "poetry==1.8.3" "xattr <1". If it succeeds then it's broken. But I'm pretty sure it's broken.

@BastianZim
Copy link
Member

Yes, it installs + poetry 1.8.3 linux_pyh707e725_0 conda-forge 167kB so that's definitely wrong.

@BastianZim
Copy link
Member

Thanks John! Will roll that back then and mark that version as broken.

@BastianZim BastianZim mentioned this pull request May 9, 2024
5 tasks
@BastianZim
Copy link
Member

New PR is in #100

@BastianZim
Copy link
Member

Package is marked as broken in conda-forge/admin-requests#991

@xylar
Copy link
Contributor

xylar commented May 9, 2024

Sorry about all this! I appreciate all of you help!

@maresb
Copy link
Contributor

maresb commented May 9, 2024

No worries @xylar, given all the amazing wide-reaching stuff you're constantly doing, it's quite remarkable how infrequently anything goes wrong!

@h-vetinari
Copy link
Member

Yeah, sorry also from my side for the misleading linter warning, and thanks everyone for fixing it!

@BastianZim
Copy link
Member

Thanks everyone for the quick help and feedback! I can only agree with Ben, at this scale, it's incredibly impressive how well everything is working. And all on volunteer time! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants