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

fix: drop support for named pipes on Windows #962

Merged

Conversation

zregvart
Copy link
Contributor

Seems that folk are having issues with uploading 0-byte files from Windows agents. This effectively removes the support for Windows for uploading from named files that, due to isFIFO returning false on Windows for named pipes created using MSYS2's mkfifo command, resorted to checking if the file size is 0 - a common trait of named pipes.

See actions/upload-artifact#281

@zregvart zregvart requested a review from a team December 10, 2021 19:12
@zregvart
Copy link
Contributor Author

cc @yacaovsnc and @konradpabjan, thanks to @lilyinstarlight for piecing this together in actions/upload-artifact#281

@zregvart zregvart force-pushed the pr/drop-fifo-support-for-windows branch from 0a77b4e to 17dc0b6 Compare December 10, 2021 19:16
gauravraa referenced this pull request Dec 13, 2021
* Updating Contributing.md + add adr details
@konradpabjan
Copy link
Contributor

Overall looks like a simple fix! 👍

@zregvart Could you add some files that have a length of 0 to this test (this will run against Windows and all other environments). This will help catch future regressions

- name: Create files that will be uploaded
run: |
mkdir artifact-path
echo ${{ env.non-gzip-artifact-content }} > artifact-path/world.txt
echo ${{ env.gzip-artifact-content }} > artifact-path/gzip.txt

@zregvart zregvart force-pushed the pr/drop-fifo-support-for-windows branch from 17dc0b6 to c0b424a Compare December 14, 2021 09:49
@zregvart zregvart requested a review from a team as a code owner December 14, 2021 09:49
@zregvart
Copy link
Contributor Author

@konradpabjan I've added the test with an empty file to the test workflow. I did notice, and it seems unrelated to this change some escaping issues on Widows. Perhaps something to look into.

Error: Provided rootDirectory D:a oolkit oolkit does not exist
Run node -e "Promise.resolve(require('./packages/artifact/lib/artifact-client').create().uploadArtifact('my-artifact-1',['artifact-path/world.txt'], 'D:\a\toolkit\toolkit'))"
  node -e "Promise.resolve(require('./packages/artifact/lib/artifact-client').create().uploadArtifact('my-artifact-1',['artifact-path/world.txt'], 'D:\a\toolkit\toolkit'))"
  node -e "Promise.resolve(require('./packages/artifact/lib/artifact-client').create().uploadArtifact('my-artifact-2',['artifact-path/gzip.txt'], 'D:\a\toolkit\toolkit'))"
  node -e "Promise.resolve(require('./packages/artifact/lib/artifact-client').create().uploadArtifact('my-artifact-3',['artifact-path/empty.txt'], 'D:\a\toolkit\toolkit'))"
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    ACTIONS_RUNTIME_URL: pipelines.actions.githubusercontent.com/fuP41n7Wlt2coACvHON7kn9nLmnoiK22qyvcSX2BhB99JO8CdP
    ACTIONS_RUNTIME_TOKEN: ***
    GITHUB_RUN_ID: 1577186317
    non-gzip-artifact-content: hello
    gzip-artifact-content: Some large amount of text that has a compression ratio that is greater than 100%. If greater than 100%, gzip is used to upload the file
    empty-artifact-content: _EMPTY_
Starting artifact upload
For more detailed logs during the artifact upload process, enable step-debugging: docs.github.com/actions/monitoring-and-troubleshooting-workflows/enabling-debug-logging#enabling-step-debug-logging
Artifact name is valid!
(node:2260) UnhandledPromiseRejectionWarning: Error: Provided rootDirectory D:a	oolkit	oolkit does not exist
    at Object.getUploadSpecification (D:\a\toolkit\toolkit\packages\artifact\lib\internal\upload-specification.js:24:15)
    at DefaultArtifactClient. (D:\a\toolkit\toolkit\packages\artifact\lib\internal\artifact-client.js:44:64)
    at Generator.next ()
    at D:\a\toolkit\toolkit\packages\artifact\lib\internal\artifact-client.js:8:71
    at new Promise ()
    at __awaiter (D:\a\toolkit\toolkit\packages\artifact\lib\internal\artifact-client.js:4:12)
    at DefaultArtifactClient.uploadArtifact (D:\a\toolkit\toolkit\packages\artifact\lib\internal\artifact-client.js:39:16)
    at [eval]:1:77
    at Script.runInThisContext (vm.js:120:18)
    at Object.runInThisContext (vm.js:309:38)
(node:2260) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:2260) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
Starting artifact upload
For more detailed logs during the artifact upload process, enable step-debugging: docs.github.com/actions/monitoring-and-troubleshooting-workflows/enabling-debug-logging#enabling-step-debug-logging
Artifact name is valid!
(node:6356) UnhandledPromiseRejectionWarning: Error: Provided rootDirectory D:a	oolkit	oolkit does not exist
    at Object.getUploadSpecification (D:\a\toolkit\toolkit\packages\artifact\lib\internal\upload-specification.js:24:15)
    at DefaultArtifactClient. (D:\a\toolkit\toolkit\packages\artifact\lib\internal\artifact-client.js:44:64)
    at Generator.next ()
    at D:\a\toolkit\toolkit\packages\artifact\lib\internal\artifact-client.js:8:71
    at new Promise ()
    at __awaiter (D:\a\toolkit\toolkit\packages\artifact\lib\internal\artifact-client.js:4:12)
    at DefaultArtifactClient.uploadArtifact (D:\a\toolkit\toolkit\packages\artifact\lib\internal\artifact-client.js:39:16)
    at [eval]:1:77
    at Script.runInThisContext (vm.js:120:18)
    at Object.runInThisContext (vm.js:309:38)
(node:6356) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:6356) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
Starting artifact upload
For more detailed logs during the artifact upload process, enable step-debugging: docs.github.com/actions/monitoring-and-troubleshooting-workflows/enabling-debug-logging#enabling-step-debug-logging
Artifact name is valid!
(node:4832) UnhandledPromiseRejectionWarning: Error: Provided rootDirectory D:a	oolkit	oolkit does not exist
    at Object.getUploadSpecification (D:\a\toolkit\toolkit\packages\artifact\lib\internal\upload-specification.js:24:15)
    at DefaultArtifactClient. (D:\a\toolkit\toolkit\packages\artifact\lib\internal\artifact-client.js:44:64)
    at Generator.next ()
    at D:\a\toolkit\toolkit\packages\artifact\lib\internal\artifact-client.js:8:71
    at new Promise ()
    at __awaiter (D:\a\toolkit\toolkit\packages\artifact\lib\internal\artifact-client.js:4:12)
    at DefaultArtifactClient.uploadArtifact (D:\a\toolkit\toolkit\packages\artifact\lib\internal\artifact-client.js:39:16)
    at [eval]:1:77
    at Script.runInThisContext (vm.js:120:18)
    at Object.runInThisContext (vm.js:309:38)
(node:4832) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:4832) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Seems that folk are having issues with uploading 0-byte files from
Windows agents. This effectively removes the support for Windows for
uploading from named files that, due to `isFIFO` returning `false` on
Windows for named pipes created using MSYS2's `mkfifo` command, resorted
to checking if the file size is 0 - a common trait of named pipes.

See actions/upload-artifact#281
@zregvart zregvart force-pushed the pr/drop-fifo-support-for-windows branch from c0b424a to fc13705 Compare December 14, 2021 20:05
Copy link
Contributor

@konradpabjan konradpabjan left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for the fix and test.

I'll work on rolling this change out

@konradpabjan konradpabjan merged commit 37f5a85 into actions:main Dec 14, 2021
@zregvart zregvart deleted the pr/drop-fifo-support-for-windows branch December 14, 2021 21:11
at-wat pushed a commit to at-wat/actions-toolkit that referenced this pull request Aug 31, 2023
Seems that folk are having issues with uploading 0-byte files from
Windows agents. This effectively removes the support for Windows for
uploading from named files that, due to `isFIFO` returning `false` on
Windows for named pipes created using MSYS2's `mkfifo` command, resorted
to checking if the file size is 0 - a common trait of named pipes.

See actions/upload-artifact#281
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.

2 participants