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

Include fs stats and mode in zip archive, fix "too many open files" error #1723

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Apr 23, 2024

This should allow Unix file permissions to be included in the zip file created by upload-artifact, fixing actions/upload-artifact#38 and probably actions/upload-artifact#37 as well.

Fixes #1722.
Fixes actions/upload-artifact#485.

This is basically #1609, rebased against the current state of the main branch. But #1609 did not include the mode property in its call to zip.append, and my research suggests that mode is needed to actually include the permissions in the zip file.

Copy link

@Bushgang1981 Bushgang1981 left a comment

Choose a reason for hiding this comment

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

This is the right one that works for me unless you know it works for me good so zip hope you figure the problem that you got going on read read you get it

@rmunn
Copy link
Contributor Author

rmunn commented Apr 29, 2024

actions/upload-artifact#485 looks relevant here, and could be easily fixed in the same PR by using archive.file instead of archive.append.

@rmunn
Copy link
Contributor Author

rmunn commented Apr 29, 2024

Commit 5f62f1e should fix actions/upload-artifact#485.

Copy link
Contributor

@SMoraisAnsys SMoraisAnsys left a comment

Choose a reason for hiding this comment

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

LGTM

Looking at file documentation this PR should fix actions/upload-artifact#485

@rmunn rmunn changed the title Include fs stats and mode in zip archive Include fs stats and mode in zip archive, fix "too many open files" error May 7, 2024
@rmunn
Copy link
Contributor Author

rmunn commented May 7, 2024

@bethanyj28 - It looks like you've been doing recent work on the packages/artifact code that this touches. Would you like to give this PR a review? It's intended to fix the seventh-most-upvoted issue in actions/upload-artifact (which has been open since 2019), so I'd like someone from GitHub to at least take a look at it. (And if you're the wrong person to ping, my apologies, and could you let me know the right person?)

@rmunn
Copy link
Contributor Author

rmunn commented Jun 4, 2024

@SrRyan @yacaovsnc You recently reviewed PRs touching this part of the code (#1678 and #1724 respectively). Could you take a look at this PR? It's intended to fix the seventh-most-upvoted issue in actions/upload-artifact and also a recent issue that prevents uploaded artifacts from containing more than about 8,000 files. The latter has a workaround, using v3, that will go away in November so there's a time limit on the bugfix.

If you're not the right people to contact about code review, would you let me know whom I should be talking to? Thank you.

@rmunn
Copy link
Contributor Author

rmunn commented Jun 14, 2024

@bdehamer - Can you tell me who is the right person to review PRs in this repo? I've asked multiple GitHub team members so far, with no response yet from anyone. This should be a pretty straightforward review, and one of the bugs it fixes is the sixth-most upvoted issue in its tracker. (Used to be seventh-most, but one was closed recently). I'm surprised it's been so hard to get a bugfix even looked at, let alone merged.

@rmunn
Copy link
Contributor Author

rmunn commented Jun 21, 2024

@konradpabjan - Your GitHub profile says you're working on GitHub Actions, and you created one of the bugs this PR is supposed to fix. Could you let me know how I can get this bugfix reviewed by a GitHub Actions team member, so that the "Too many open files" fix can finally be merged?

@ntindle
Copy link

ntindle commented Jul 12, 2024

https://github.com/orgs/actions/people one of these people will probably need pinged

@rmunn
Copy link
Contributor Author

rmunn commented Jul 20, 2024

@joshmgross - Could I ask you to take a look at this PR? It's intended to fix the sixth-most-upvoted issue in actions/upload-artifact and also a recent issue that prevents uploaded artifacts from containing more than about 8,000 files. The latter has a workaround, using v3, that will go away in November so there's a time limit on the bugfix.

P.S. I apologize for the random ping, but as you'll see from the comment history, this PR has been open for three months and I've had no success yet in getting anyone from the GitHub Actions team to look at this bugfix. Even a "Sorry, we don't accept outside PRs for this code" message would be fine, as long as some bugfix for actions/upload-artifact#485 goes in.

@joshmgross
Copy link
Member

👋 Hey @rmunn,

I'll check internally to see what we can do here - my best guess is it will be safer to separate out the bug fix here from the feature to support file permissions. We do accept outside PRs, but generally speaking we're a bit limited on new features as that increases the maintenance burden of the action (every new feature is something we will have to support in perpetuity).

@rmunn
Copy link
Contributor Author

rmunn commented Jul 23, 2024

Thanks for responding. This PR was originally based around supporting file permissions, which is a feature request that's more than four years old at this point (actions/upload-artifact#38, actions/download-artifact#14). I then added the "too many open files" fix because I was already touching the same code, and why increase the maintenance burden by asking an overworked team to review two separate PRs?

But if, in fact, the maintenance burden would be reduced by splitting the PR in two, then I'll do it that way. I'll open a new PR for the fix to actions/upload-artifact#485, and keep this one focused on its original intent, the .zip file permissions loss. (Which I consider a bugfix, not a new feature, but at least it has a workaround: create a .tar file and upload that. Though that then runs into actions/upload-artifact#426 and actions/upload-artifact#39, but those are annoyance-level bugs rather than blockers).

@rmunn
Copy link
Contributor Author

rmunn commented Jul 23, 2024

@joshmgross - I've created #1771 for the EMFILE fix without the zip file permissions bugfix.

@robherley
Copy link
Contributor

@rmunn Do you mind fixing the conflicts, and then we can review? To properly address the EMFILE fix, we need to check the stats (for symlinks), see my comment here: actions/upload-artifact#485 (comment)

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.

Unix file permissions not stored in zip file [bug] EMFILE: too many open files
6 participants