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

ci: fix name of artifacts in deploy-cpp #2940

Merged
merged 2 commits into from
Jan 12, 2024
Merged

Conversation

agoose77
Copy link
Collaborator

This was done on-the-fly, so it might have mistakes. I'll give it another look before merging.

The changes to upload-artifact include breaking merging of same-name artifacts (the default behaviour if artifacts are unnamed). This PR adds explicit names, and downloads artifacts by a pattern.

@jpivarski
Copy link
Member

3081 Segmentation fault      (core dumped) python -m pytest -vv -rs tests --cov=awkward --cov-report=term --cov-report=xml

is an intermittent segfault in PyPy that we're monitoring now. I restarted the jobs and it will probably pass. (Obviously, this isn't a good situation for anyone using PyPy; we need to figure out what's causing the segfault, at least because there's a slight chance it's also a silent undefined behavior in CPython.)

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Thanks! (This is un-reverting c13c091.)

This will probably pass tests before the awkward-cpp deployment is done; I'll merge it to try out the updated deploy.yml when deploying awkward 2.5.2. It looks like the most substantial change is in deploy-cpp.yml, though.

If (after all of the new-release dust settles) I run deploy-cpp.yml without "deploy to PyPI", will it be a sufficient test of this fix? If so, I'll do that, so that it won't be a surprise the next time we need an awkward-cpp.

Copy link

codecov bot commented Jan 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f74bf8d) 81.88% compared to head (a67c0ed) 81.88%.

Additional details and impacted files

@jpivarski
Copy link
Member

When https://github.com/scikit-hep/awkward/actions/runs/7505629752 is done, I'll merge this.

@jpivarski jpivarski merged commit ee63f6e into main Jan 12, 2024
39 checks passed
@jpivarski jpivarski deleted the agoose77/ci-fix-name-artifacts branch January 12, 2024 18:36
@jpivarski
Copy link
Member

Hmm. That didn't do it because there's an asterisk in the name, where I think you wanted it in the pattern only:

Run actions/upload-artifact@v4
  with:
    name: awkward-wheels-ubuntu-latest-auto64-pp*
    path: wheelhouse/*.whl
    if-no-files-found: warn
    compression-level: 6
  env:
    SOURCE_DATE_EPOCH: 1705076015
    pythonLocation: /opt/hostedtoolcache/Python/3.11.7/x64
    PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.11.7/x64/lib/pkgconfig
    Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.7/x64
    Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.7/x64
    Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.7/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.11.7/x64/lib
With the provided path, there will be 3 files uploaded
Error: The artifact name is not valid: awkward-wheels-ubuntu-latest-auto64-pp*. Contains the following character:  Asterisk *
          
Invalid characters include:  Double quote ", Colon :, Less than <, Greater than >, Vertical bar |, Asterisk *, Question mark ?, Carriage return \r, Line feed \n, Backslash \, Forward slash /
          
These characters are not allowed in the artifact name due to limitations with certain file systems such as NTFS. To maintain file system agnostic behavior, these characters are intentionally not allowed to prevent potential problems with downloads on different file systems.

@jpivarski
Copy link
Member

From the name awkward-wheels-ubuntu-latest-auto64-pp*, it must be this one:

- name: Upload wheels
uses: actions/upload-artifact@v4
with:
name: awkward-wheels-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.build }}
path: wheelhouse/*.whl

It's picking up this "pp*" as the build name:

build: ["cp*", "pp*"]

rather than the file or directory that it matches to.

But this should have the same multiplicity as the number of CI tasks, not the number of files in the directory that the wildcard matches to, right? So in that case, it could be fixed by (a) stripping off the "*" using string manipulation or (b) not using a wildcard in the definition of the matrix.build variable, but adding a wildcard after its use in defining this environment variable:

- uses: pypa/cibuildwheel@v2.16.2
env:
CIBW_BUILD: ${{ matrix.build }}

(Both values for matrix.build have a wildcard in the same place.)

@jpivarski
Copy link
Member

jpivarski commented Jan 12, 2024

I'll implement that in a non-PR commit, and then test it in deploy-cpp.yml now: 327b1bf.

@jpivarski
Copy link
Member

jpivarski commented Jan 12, 2024

So far, I think that's working (https://github.com/scikit-hep/awkward/actions/runs/7506459981), but I'll let it run all the way through to be sure.

Edit: It's done, and it's fine.

@agoose77
Copy link
Collaborator Author

Thanks Jim! I noticed the failure on my phone, and I appreciate you fixing it first! Rebuilding the CIBW_BUILD variable looks fine to me — easy to change this down the road.

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

2 participants