fix: Use .zip in archive path
#119
Conversation
|
Thanks @dnys1! Would you mind resolving conflicts? Then we'll take a look. |
|
@mit-mit Done 😄 |
There was a problem hiding this comment.
Looks good, thanks!
Can you also add a new changelog entry to the changelog file? You can start a new v1.6.5-wip entry.
The diff of index.mjs is quite large. I believe this is due to the version of Node I'm using (20.7.0). Happy to use a different version to keep the diff small, but wasn't sure which one is currently used. This could be noted in package.json (under engines), if desired, to ensure alignment with external contributions.
fwiw I'm using v20.6.1. If you think that this will reduce diffs then PRs here would be welcome. We're mostly treating the dist/index.mjs file as opaque - we know it works if all the CI tests pass. Do you think specifying a specific version of node would help? I'd be worried about then remembering to rev that version periodically. Plus, many of the diffs in the file may be from the npm packages in use?
lib/main.dart
Outdated
| await promiseToFuture<String>(toolCache.downloadTool(url)); | ||
| final archivePath = path.join( | ||
| // `RUNNER_TEMP` variable is guaranteed to be present. | ||
| // https://github.com/actions/toolkit/blob/5430c5d84832076372990c7c27f900878ff66dc9/packages/tool-cache/src/tool-cache.ts#L756 |
There was a problem hiding this comment.
| // https://github.com/actions/toolkit/blob/5430c5d84832076372990c7c27f900878ff66dc9/packages/tool-cache/src/tool-cache.ts#L756 | |
| // https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables |
Fixes dart-lang#118 Ensures that downloads of the Dart SDK zip file carry the `.zip` extension so that subsequent calls to `Expand-Archive` on Windows succeed. ## Notes - The diff of `index.mjs` is quite large. I believe this is due to the version of Node I'm using (`20.7.0`). Happy to use a different version to keep the diff small, but wasn't sure which one is currently used. This could be noted in `package.json` to ensure alignment with external contributions.
dnys1
force-pushed
the
fix/windows-extract
branch
from
ff3f5c4
to
388f8d3
Compare
There was a problem hiding this comment.
Hey @devoncarew, apologies for the delay on this.
Can you also add a new changelog entry to the changelog file? You can start a new
v1.6.5-wipentry.
Added 👍
fwiw I'm using v20.6.1. If you think that this will reduce diffs then PRs here would be welcome. We're mostly treating the dist/index.mjs file as opaque - we know it works if all the CI tests pass. Do you think specifying a specific version of node would help? I'd be worried about then remembering to rev that version periodically. Plus, many of the diffs in the file may be from the npm packages in use?
If you're okay with treating index.mjs as opaque, I think that's an equally good option. There could still be benefit in adding an engine reference, which would just be to ensure older (perhaps, deprecated) versions of Node are not used by mistake. In this case, the constraint could be more lax, e.g. >=20.
lib/main.dart
Outdated
| await promiseToFuture<String>(toolCache.downloadTool(url)); | ||
| final archivePath = path.join( | ||
| // `RUNNER_TEMP` variable is guaranteed to be present. | ||
| // https://github.com/actions/toolkit/blob/5430c5d84832076372990c7c27f900878ff66dc9/packages/tool-cache/src/tool-cache.ts#L756 |
|
I'm blocked by this issue. I don't have access to directly contribute to this PR, so I opt to take the commits from this PR, and created a new PR with one more additional commit to fix the build issue: #134 |
|
Thanks @ntkme I hadn't seen the build failure. I've pushed a new commit which runs the formatter, rebuilds, and signs the JS using |
|
@dnys1 Thanks for the update. @devoncarew Can you please take another look and see if we can get this merged? |
|
I'll leave for @devoncarew to hit the merge button! |
|
Thanks for the contribution! |
Fixes #118
Ensures that downloads of the Dart SDK zip file carry the
.zipextension so that subsequent calls toExpand-Archiveon Windows succeed.Notes
index.mjsis quite large. I believe this is due to the version of Node I'm using (20.7.0). Happy to use a different version to keep the diff small, but wasn't sure which one is currently used. This could be noted inpackage.json(under engines), if desired, to ensure alignment with external contributions.Contribution guidelines:
dart format.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.