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

docs: update examples for v4 of upload-artifact and download-artifact actions #1705

Merged
merged 3 commits into from Dec 20, 2023

Conversation

alugowski
Copy link
Contributor

See discussion: #1699

@alugowski
Copy link
Contributor Author

alexlancaster added a commit to alexlancaster/pypop that referenced this pull request Dec 19, 2023
change syntax so that artifact names are unique according to: pypa/cibuildwheel#1705
alexlancaster added a commit to alexlancaster/pypop that referenced this pull request Dec 19, 2023
* change syntax so that artifact names are unique according to: pypa/cibuildwheel#1705
Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you for putting this together. @henryiii I see you're across this so would you mind double-checking this too?

@henryiii
Copy link
Contributor

Looks mostly good, just two thoughts:

  1. We could make the names generically, using cibw-${{ github.job }}-${{ strategy.job-index }}.
  2. We could avoid downloading all artifacts, and only download ours using pattern: cibw-*.

I think the current PR is probably simplest, as usually (and in this example) there aren't extra artifacts to worry about. We could mention this somewhere, though.

@njzjz
Copy link
Contributor

njzjz commented Dec 19, 2023

We could make the names generically, using cibw-${{ github.job }}-${{ strategy.job-index }}.

I tried this, but the name looks too generic when I want to download the wheel for a specific platform.

image

It's hard to know which one is built for Linux.

@alugowski
Copy link
Contributor Author

alugowski commented Dec 20, 2023

Would cibw-wheels-${{ matrix.os }}-${{ strategy.job-index }} and cibw-sdist be a good complexity/clarity/uniqueness compromise?

I thought just matrix.os is enough, but the GH docs use that as the example and I've seen comments complaining, as if they didn't know it was just a suggestion. I think the places where github.job would help disambiguate are few, and those folks would know to add it.

Also I just noticed the examples/ directory that has like 4 workflows that should be updated too. Once a call is made on this I can update those.

@alugowski
Copy link
Contributor Author

1. We could make the names generically, using `cibw-${{ github.job }}-${{ strategy.job-index }}`.

Based on feedback above I went with cibw-wheels-${{ matrix.os }}-${{ strategy.job-index }}, but happy to change.

2. We could avoid downloading all artifacts, and only download ours using `pattern: cibw-*`.

Done.

I also updated the example .yml files.

@joerick
Copy link
Contributor

joerick commented Dec 20, 2023

I like the renamed artifacts with the cibw prefix, it also make it clearer what 'merge-multiple' means in the example.

I'll let @henryiii have another look, but LGTM.

Copy link
Contributor

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

LGTM!

@henryiii henryiii merged commit 2c49742 into pypa:main Dec 20, 2023
18 checks passed
@alexlancaster
Copy link

alexlancaster commented Dec 21, 2023

The PR seems to be missing an update to the GitHub action in the "Example setup " in https://github.com/pypa/cibuildwheel/blob/main/README.md ?

@alugowski
Copy link
Contributor Author

The PR seems to be missing an update to the GitHub action in the "Example setup " in https://github.com/pypa/cibuildwheel/blob/main/README.md ?

Good catch.

#1708

xgarrido added a commit to simonsobs/pspy that referenced this pull request Dec 21, 2023
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.

None yet

5 participants