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 Windows path registration #6

Merged
merged 21 commits into from
Feb 20, 2023
Merged

Conversation

Spiker985
Copy link
Member

Add the Pulsar install directory, and ppm bin directory to the user path - this should hopefully allow downstream steps to be able to shorthand pulsar instead of the full path

Add the Pulsar install directory, and ppm bin directory to the user path - this should hopefully allow downstream steps to be able to shorthand `pulsar` instead of the full path
Because YAML
- Revert back to machine path over user path
- Use `[System.Environment]SetEnvironmentVariable` and `[System.Environment]GetEnvironmentVariable` methods
- `/currentuser` picks the Appdata\Local location instead of Program Files
- `/S` is the silent flag
- `-wait` is needed otherwise we'd move on before the editor was installed
Just to be absolutely sure that it knows it's a directory
@Spiker985 Spiker985 self-assigned this Feb 10, 2023
@Spiker985
Copy link
Member Author

I had to update the test to split the windows test step from the Linux and macOS step. I don't know if that will have an effect on downstream steps. Especially when paired with xvfb.

@Spiker985 Spiker985 marked this pull request as ready for review February 11, 2023 08:44
@Spiker985 Spiker985 requested a review from a team February 11, 2023 08:44
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

I'm approving because the changes look solid, and based on our own tests seem to work.

Although prior to merging I think it'd be important to test this PR's changes on an actual package to see what kind of affect it has on testing for Windows.

Then we can use that to determine if this PR includes breaking changes or not, since otherwise I think we should get this one merged no matter what.

Thanks a ton for the contribution, this looks awesome!

@Spiker985
Copy link
Member Author

Currently trying this out on a branch for x-terminal-reloaded. According to the GitHub Actions docs for using versioned actions you can actually target a branch - which I've done in my most recent commit

@Spiker985
Copy link
Member Author

So, it seems like it was simpler than previously thought to get the Windows PATH fixed for CI - just had to find the right documentation which is definitely easier said than done

.github/workflows/test.yml Outdated Show resolved Hide resolved
Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Other than wanting the whitespace-only change nullified on the test.yml file, I can register a "lite-approve" on this.

I don't feel qualified to properly approve since I don't understand whether or how the GITHUB_PATH stuff all works. But if you've tested this, I appreciate that. So I am lightly in favor of merging this if it moves this repo forward.

Spiker985 and others added 2 commits February 18, 2023 19:54
As of 1.102.0, macOS is now signed
@Spiker985
Copy link
Member Author

When I looked, I found this which I think is our macOS problem when actually running on CIs such as pulsar-edit/snippets#10 and Spiker985/x-terminal-reloaded#14

- Validate `/usr/local/bin` exists, and make it otherwise
- Change the `pulsar` symlink to `pulsar.sh`
- Add a commented symlink for ppm (just in case) - however, `pulsar -p` or `pulsar --package` should handle fine without it
- Change to recommending `coactions/setup-xvfb` over `GabrielBB/xvfb-action` as the latter appears unmaintained, and needs updated
- Preemptively update the version to match the expected release version
@Spiker985 Spiker985 merged commit 8c5f821 into main Feb 20, 2023
@Spiker985 Spiker985 deleted the s985-update-win-path-registration branch February 20, 2023 05:36
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.

3 participants