include virtual packages in hash contents #4606

isuruf · 2022-10-29T20:58:20Z

Description

Noarch packages that use virtual packages have the virtual packages added to the hash contents of the package.
This facilitates the building of noarch packages multiple times for different platforms with platform specific dependencies.

If we build noarch packages with diverging run dependencies for different platforms, the hash in the build string stayed the same prior to this PR, i.e., the built variants got the exact same build strings and thus package file names didn't change.
With this PR we can build different variants for, e.g., __linux/__osx/__win, and get distinguished, non-clashing package file names.

Resolves #4861.

Checklist - did you ...

Add a file to the news directory (using the template) for the next release's release notes?
Add / update necessary tests?
~~Add / update outdated documentation?~~

jaimergp · 2022-10-31T07:48:11Z

For context, we need this as part of the work being done at conda-forge/conda-forge.github.io#1840, following approach documented at conda-forge/conda-forge.github.io#1839

jakirkham · 2022-11-11T21:07:52Z

cc @jezdez

mbargull

LGTM; just small suggestions for extending the test a little bit.

mbargull · 2022-11-17T05:59:15Z

tests/test-recipes/metadata/_noarch_with_platform_deps/meta.yaml

+    - colorama   # [win]
+    - __win      # [win]
+    - appnope    # [osx]
+    - __osx      # [osx]


Suggested change

- __osx # [osx]

- __osx # [osx]

- __linux # [ppc64le]

I've gone with __linux # [linux]. I don't think it's a good idea to use ppc64le as the selector here as the selector and the virtual package differs.

Yes, semantically the recipe doesn't makes sense. However, I changed it that way to let the test cover more cases, i.e., without it, we don't test whether the hash changes or not depending on if we have a virtual package dep on the same platform (OS) or not.

Do we have virtual architecture packages?

Does the archspec virtual package count?

Interesting. How would this be used? Maybe something like this?

Suggested change

- __osx # [osx]

- __osx # [osx]

- __archspec * ppc64le # [ppc64le]

Tried it locally and it works!

Patched the test to cover for John's comment in this other suggestion. Would that work @mbargull?

That's only true when archspec is not installed and is a fallback. Otherwise you get values like power9, power10 etc.

Hm, it passes with or without archspec in my dev env 🤷

tests/test_api_render.py

tests/test-recipes/metadata/_noarch_with_no_platform_deps/meta.yaml

jezdez · 2022-11-17T19:56:12Z

I've just updated the PR description with what had been written in the news file contained in this PR and I don't understand why this wasn't done initially when this was opened.

It would help the review process, especially for the people relatively new to the code base like myself, compared to those that have plenty of experience working with it every day.

To be clear, it adds stop energy to the review process if there is:

no issue related to the proposed change or
the pull request description not explaining why the change is needed or
the linked information being more pull requests elsewhere ("Packages that can be noarch" and "Document OS-specific approaches for noarch packages")

If this feature request is important for conda-forge, it must be filed as an issue in this repo, to be referenced during the code review, not the least for future code archeologists and consensus finding.

I understand that filing issues has probably been dismissed as not helpful in the past, for various reasons (read: bad maintenance patterns), but I'd appreciate your help in making it easier for me and my colleagues to review your suggested changes. That would seriously help to unblock you as well I think.

Thank you for your contributions -- as always -- @isuruf!

conda_build/metadata.py

jakirkham · 2022-12-13T19:29:35Z

What are the next steps here?

jaimergp · 2023-01-05T15:27:05Z

One of the tests added in this PR is failing with a setattr problem in the config. Maybe a bad key somewhere?

jakirkham · 2023-02-16T19:25:09Z

This seems to be lingering a bit. How can we help move this forward?

isuruf · 2023-02-16T19:33:54Z

I don't have the time to work on this PR. Anyone wants to take over?

jakirkham · 2023-02-16T19:46:50Z

No worries. Should add that question wasn't directed at you specifically, but to this group as a whole. It's not obvious what still needs to be done to merge. For example there are outdated review comments above. Another review might clarify what's still needed.

beeankha · 2023-02-16T20:42:59Z

Hi @isuruf and @jakirkham ! Several conda-core dev team members are out of office currently but I'll bring this up early next week to see if we can get this moving (or at least get a timeline/update for you). Apologies for the delay!

jakirkham · 2023-02-16T20:46:02Z

Thanks Bianca! 🙏

No worries. Things slip through the cracks sometimes.

Just want to make sure that we pick this one back up if possible.

isuruf · 2023-02-16T20:54:43Z

I think @jezdez's comment still applies

If this feature request is important for conda-forge, it must be filed as an issue in this repo, to be referenced during the code review, not the least for future code archeologists and consensus finding.

I understand that filing issues has probably been dismissed as not helpful in the past, for various reasons (read: bad maintenance patterns), but I'd appreciate your help in making it easier for me and my colleagues to review your suggested changes. That would seriously help to unblock you as well I think.

So, someone should open an issue with a description. @mbargull added a good description for the top post here. We can copy that and add more context if needed.

h-vetinari · 2023-05-02T07:08:41Z

So, someone should open an issue with a description.

Done: #4861

jezdez

Thanks for elaborating in the ticket and providing the additional context, everyone!

Co-authored-by: Marcel Bargull <mbargull@users.noreply.github.com>

jezdez · 2023-05-03T20:15:53Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

tests/test_api_render.py

Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>

beeankha · 2023-05-04T14:24:46Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

tests/test_api_render.py

jezdez · 2023-05-05T08:49:31Z

@mbargull Any chance to re-review this?

Tests have been extended

jakirkham · 2023-05-05T17:32:39Z

Thanks all! 🙏

isuruf mentioned this pull request Oct 29, 2022

Document OS-specific approaches for noarch packages conda-forge/conda-forge.github.io#1839

Merged

2 tasks

conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Oct 29, 2022

isuruf force-pushed the virtual_hash branch from 6bc7f22 to 8f31fca Compare November 1, 2022 21:28

isuruf marked this pull request as ready for review November 1, 2022 21:28

isuruf force-pushed the virtual_hash branch from db191ea to 9dda598 Compare November 1, 2022 23:17

BastianZim mentioned this pull request Nov 8, 2022

Make package noarch conda-forge/spyder-kernels-feedstock#86

Merged

5 tasks

isuruf force-pushed the virtual_hash branch from 9dda598 to 50f6766 Compare November 11, 2022 19:21

mbargull previously requested changes Nov 17, 2022

View reviewed changes

jezdez reviewed Nov 17, 2022

View reviewed changes

conda_build/metadata.py Show resolved Hide resolved

jaimergp mentioned this pull request Mar 16, 2023

Gallery of toy recipes that implement "known conda-build patterns" conda-forge/conda-forge.github.io#1908

Open

h-vetinari mentioned this pull request May 2, 2023

Include virtual packages in hash contents #4861

Closed

2 tasks

jezdez previously approved these changes May 3, 2023

View reviewed changes

jezdez requested review from mbargull and chenghlee May 3, 2023 18:09

jezdez force-pushed the virtual_hash branch 2 times, most recently from 4c9d5b3 to 97c2240 Compare May 3, 2023 19:55

isuruf added 2 commits May 3, 2023 22:13

include virtual packages in hash contents

b41b4ea

add a test

4d8dabb

isuruf and others added 7 commits May 3, 2023 22:13

add news

f4d4138

underscore in tests

5c00453

fix typo

7861537

Extend test

7d897ec

Co-authored-by: Marcel Bargull <mbargull@users.noreply.github.com>

Keep all linux equal

5dc6c3e

Fix linux recipe

b43ecb7

Update conda_build/metadata.py

0252216

Co-authored-by: Marcel Bargull <mbargull@users.noreply.github.com>

jezdez force-pushed the virtual_hash branch from 97c2240 to 0252216 Compare May 3, 2023 20:13

[pre-commit.ci] auto fixes from pre-commit.com hooks

41ff1e7

for more information, see https://pre-commit.ci

pre-commit-ci bot dismissed jezdez’s stale review via 41ff1e7 May 3, 2023 20:16

jaimergp reviewed May 4, 2023

View reviewed changes

tests/test_api_render.py Outdated Show resolved Hide resolved

Update tests/test_api_render.py

df0edb5

Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

c19d841

for more information, see https://pre-commit.ci

beeankha previously approved these changes May 4, 2023

View reviewed changes

jaimergp reviewed May 4, 2023

View reviewed changes

tests/test_api_render.py Outdated Show resolved Hide resolved

Add archspec and update test accordingly

891a5fc

beeankha dismissed their stale review via 891a5fc May 4, 2023 17:08

Merge branch 'main' into virtual_hash

556442d

beeankha approved these changes May 4, 2023

View reviewed changes

jezdez approved these changes May 5, 2023

View reviewed changes

beeankha merged commit ccc3e81 into conda:main May 5, 2023

isuruf deleted the virtual_hash branch May 5, 2023 17:38

github-actions bot added the locked [bot] locked due to inactivity label May 5, 2024

github-actions bot locked as resolved and limited conversation to collaborators May 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

include virtual packages in hash contents #4606

include virtual packages in hash contents #4606

isuruf commented Oct 29, 2022 •

edited by jezdez

Loading

jaimergp commented Oct 31, 2022

jakirkham commented Nov 11, 2022

mbargull left a comment

mbargull Nov 17, 2022

isuruf Nov 17, 2022

mbargull Nov 17, 2022

jakirkham Jan 3, 2023

jezdez Jan 3, 2023

jakirkham Jan 3, 2023

jaimergp May 4, 2023

jaimergp May 4, 2023

isuruf May 4, 2023

jaimergp May 4, 2023

jezdez commented Nov 17, 2022

jakirkham commented Dec 13, 2022

jaimergp commented Jan 5, 2023

jakirkham commented Feb 16, 2023

isuruf commented Feb 16, 2023

jakirkham commented Feb 16, 2023

beeankha commented Feb 16, 2023

jakirkham commented Feb 16, 2023

isuruf commented Feb 16, 2023 •

edited

Loading

h-vetinari commented May 2, 2023

jezdez left a comment

jezdez commented May 3, 2023

beeankha commented May 4, 2023

jezdez commented May 5, 2023

jakirkham commented May 5, 2023

	- __osx # [osx]
	- __osx # [osx]
	- __archspec * ppc64le # [ppc64le]

include virtual packages in hash contents #4606

include virtual packages in hash contents #4606

Conversation

isuruf commented Oct 29, 2022 • edited by jezdez Loading

Description

Checklist - did you ...

jaimergp commented Oct 31, 2022

jakirkham commented Nov 11, 2022

mbargull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jezdez commented Nov 17, 2022

jakirkham commented Dec 13, 2022

jaimergp commented Jan 5, 2023

jakirkham commented Feb 16, 2023

isuruf commented Feb 16, 2023

jakirkham commented Feb 16, 2023

beeankha commented Feb 16, 2023

jakirkham commented Feb 16, 2023

isuruf commented Feb 16, 2023 • edited Loading

h-vetinari commented May 2, 2023

jezdez left a comment

Choose a reason for hiding this comment

jezdez commented May 3, 2023

beeankha commented May 4, 2023

jezdez commented May 5, 2023

jakirkham commented May 5, 2023

isuruf commented Oct 29, 2022 •

edited by jezdez

Loading

isuruf commented Feb 16, 2023 •

edited

Loading