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

Add PowerShell command to setup Windows environment #100

Merged
merged 24 commits into from
Feb 3, 2025

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Jun 3, 2024


  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

$(docker_run) koalaman/shellcheck hooks/** bin/** --exclude=SC1071
$(docker_run) koalaman/shellcheck $(shell find hooks bin -type f -not -name "*.ps1") --exclude=SC1071
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

(I wonder if there's a shellcheck equivalent for PowerShell btw… though probably only a thing to look at when we'll start tacking #101 more officially/broadly?)

Copy link
Contributor

@AliSoftware AliSoftware Jun 25, 2024

Choose a reason for hiding this comment

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

I wonder if there's a shellcheck equivalent for PowerShell btw…

Found this… in case we want to look into it at some point

Comment on lines -14 to +18
context 'All Commands Should Be Executable' do
Dir.children('bin').map { |f| File.new(File.join('bin', f)) }.each do |file|
it file.path do
expect(file.stat.executable?).to be true
context 'All Unix Commands Should Be Executable' do
Dir
.children('bin')
# Ignore Windows PowerShell scripts
.reject { |f| f.end_with?('.ps1') }
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@dangermattic
Copy link

dangermattic commented Feb 3, 2025

1 Warning
⚠️ Please add an entry in the CHANGELOG.md file to describe the changes made by this PR

Generated by 🚫 Danger

@mokagio mokagio changed the base branch from mokagio/npm_ci to trunk February 3, 2025 05:22
Even if one runs `chmod u+x` on the file, the test fails.
Anyways, we don't care about PowerShell being executable from a Unix
point of view, because they should be used only under Windows.
I usually use Vim for Ruby files, where I have RuboCop configured.
Today, I used VS Code, where I _should have_ configured RuboCop but
evidently have not.
Context: I tried calling `add_ssh_key_to_agent` straight from the
Windows environment but it didn't work.
Seems like all the flags did not make any difference in terms of whether
the symlink creation was attempted.

E.g. This build failed with the same symlink error as before, despite
having the flags
https://buildkite.com/automattic/beeper-desktop/builds/2907#0191982e-d1c5-402f-99be-2299321ccbe6
@mokagio mokagio force-pushed the mokagio/windows-utils branch from 034091f to 43653a0 Compare February 3, 2025 05:24
@mokagio mokagio force-pushed the mokagio/windows-utils branch from 43653a0 to 647299c Compare February 3, 2025 05:26
@mokagio mokagio marked this pull request as ready for review February 3, 2025 05:27
@mokagio
Copy link
Contributor Author

mokagio commented Feb 3, 2025

After months of this sitting here approved but with the tests failing, I finally got down to fixing it.

The reason for the test failure, i.e. for the new ps1 not being added to the $PATH when running in CI (but working on my local, despite both using Docker) was that... the ps1 was not marked as executable. It took ChatGPT to make me realize this. Thanks Open AI, I guess 😅

See how 647299c fixes the make test CI failure.

I did a bit of force pushing to rebase this branch on trunk instead of #96 so it can be merged on trunk directly.

Given the only effective change since @AliSoftware approved this was changing the executable status of the ps1 script, I'm going to cash in the approval and merge this so I can ship a new version.

The context for getting onto this change is that we'll need to set up Windows CI on the new Day One Electron app soon. I'm ready to jump on it, but I'd prefer if someone else had a go, to reduce the bus factor. With that in mind, it seemed appropriate to at least help and remove the oddity of having to use a custom branch in the CI setup.

@mokagio mokagio merged commit ba8e178 into trunk Feb 3, 2025
13 checks passed
@mokagio mokagio deleted the mokagio/windows-utils branch February 3, 2025 05:33
@mokagio mokagio restored the mokagio/windows-utils branch February 3, 2025 06:50
@mokagio
Copy link
Contributor Author

mokagio commented Feb 3, 2025

I restored the branch so builds referencing it won't fail while we transition them to a stable version.

@AliSoftware
Copy link
Contributor

@mokagio you might also want to add a CHANGELOG entry for this PR in a follow-up on before doing the next tag/release of this plugin

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