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

update @actions/core to 1.10.0 #17

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

compnerd
Copy link
Contributor

GitHub is working towards deprecating set-output in favour of a file based approach. Update the @actions/core dependency to make avail of the new implementation which should hopefully eliminate the deprecation warning.

GitHub is working towards deprecating `set-output` in favour of a file
based approach.  Update the `@actions/core` dependency to make avail of
the new implementation which should hopefully eliminate the deprecation
warning.
@compnerd
Copy link
Contributor Author

@seanmiddleditch I've recreated the PR (was originally #16) to rename the branch; I'm unsure if this project is now considered abandoned, but I've renamed the branch to allow others to use that as an alternate source to deal with the deprecation until we can sort out how to get this merged.

compnerd and others added 13 commits January 26, 2023 07:38
Refresh the build to allow direct use and avoid the warnings from the
deprecation of `set-action`.
@actions/core is a core dependency, not a developer dependency. Reflect
this in the manifest.
Refresh the built image with the latest changes.
Adjust the invocation of `vswhere` to avoid the version information for the tool.
This introduces a `verbose` option that allows us to easily locate the information about the installation in use without having to infer that.
The package was originally licensed under the MIT license, and the fork did not change that. Adjust the package metadata to reflect this.
Include a note about the new `verbose` option. Update the example to reference the fork, and pin the checkouts action.
When logging in verbose mode the information about the VS instance, we
need to join the lines as the output is split on each line. This should
clean up the output.
- uses: actions/checkout@master
- uses: seanmiddleditch/gha-setup-vsdevenv@master
- uses: actions/checkout@v4
- uses: compnerd/gha-setup-vsdevenv@main
Copy link
Owner

Choose a reason for hiding this comment

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

Can you revert this change for the PR so it still points to this repo?


if (verbose) {
const args = [
'-nologo',
Copy link
Owner

Choose a reason for hiding this comment

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

The first three args are the same as in vswhereArgs; perhaps a separate prefix list of common options should be defined?

'-latest',
'-products', '*',
].concat(requiresArg)
const details = spawn(vswherePath, args, { encoding: 'utf8' })
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps just set the stdio args option to [ 'ignore', 'inherit', 'inherit' ] and then not bother with details ? That'll be more efficient (no buffering of input just to dump it back out to standard output), though that probably doesn't matter in any real way here.

@@ -51,7 +64,7 @@ try {
if (hostArch != '')
vsDevCmdArgs.push(`-host_arch=${hostArch}`)
if (toolsetVersion != '')
vsDevCmdArgs.push(`-vcvars_vers=${toolsetVersion}`)
vsDevCmdArgs.push(`-vcvars_ver=${toolsetVersion}`)
Copy link
Owner

Choose a reason for hiding this comment

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

Just to verify, is this a valid bugfix, or a new typo?

@@ -4,14 +4,16 @@
"description": "Setup VS dev environment for GitHub Actions",
"main": "dist/index.js",
"author": "Sean Middleditch",
"license": "CC0",
"license": "MIT",
Copy link
Owner

Choose a reason for hiding this comment

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

Please revert this; I do not want to require attribution for folks who fork/reuse code this trivial.

@seanmiddleditch
Copy link
Owner

There's a few different changes here. I'd somewhat prefer them split out into separate PR, e.g. the verbose flag and any bug fixes.

(Repo isn't fully abandoned; I'm just not a very active maintainer, owning to not being paid to do this and trying to spend more time on non-computer-related hobbies.)

@compnerd
Copy link
Contributor Author

Ah, Thanks @seanmiddleditch! Sorry, I assumed that it was abandoned and starting making more changes. I can certainly split out most of them and send them back here if you are still interested.

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.

4 participants