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

Remove rimraf devdep #156

Merged
merged 1 commit into from
Oct 18, 2024
Merged

Remove rimraf devdep #156

merged 1 commit into from
Oct 18, 2024

Conversation

bcomnes
Copy link
Owner

@bcomnes bcomnes commented Oct 18, 2024

Remove rimraf as a dev dep. One less thing to update.

@bcomnes bcomnes changed the title Remove rimraf Remove rimraf devdep Oct 18, 2024
Copy link
Collaborator

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

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

As long as the tests still pass on Windows I’m 🙌

@bcomnes bcomnes merged commit c661ffc into master Oct 18, 2024
12 checks passed
@bcomnes bcomnes deleted the rm-rf-rimraf branch October 18, 2024 14:26
@TWiStErRob
Copy link

Hmm, I have a feeling that the docs should keep the rimraf there so the "recommendation" is to support multiplatform builds?

@bcomnes
Copy link
Owner Author

bcomnes commented Oct 22, 2024

I know github action windows runners support rm -rf directly now, is that a gha specific feature?

Alternatively could we instead recommend windows users use wsl for multiplatform builds?

My goal is to eliminate random one off solutions like rimraf when there are better platform solutions available now.

@TWiStErRob
Copy link

I'm not sure it's GHA only. I'll check what's available to confirm if this actually affects pure Windows (non-WSL) developer machines.

@XhmikosR
Copy link
Contributor

FYI this breaks native Windows cmd and requires a unix environment, so WSL, MSYS, etc.

@bcomnes
Copy link
Owner Author

bcomnes commented Oct 24, 2024

Wsl should work. Posh has rm -rf now right? Git bash is also a good option on windows. GitHub action windows runners have rm -rf now. Besides this is just used in testing and docs, people are free to use rimraf as needed. I just don't want to keep updating this dep since it doesn't seem that critical here.

@voxpelli
Copy link
Collaborator

@XhmikosR Do you know how this can be the case then?

I know github action windows runners support rm -rf directly now, is that a gha specific feature?

Are GitHub:s runners not representative of a normal windows box? Does it run a different version of windows to what you do?

Unless there’s a way to test and verify that it works or break then it’s hard to know

@XhmikosR
Copy link
Contributor

@voxpelli the default is to use bash even on Windows, hence why this doesn't fail on CI. So, no, it's not default regarding the shell. But believe me, it does fail on Windows native cmd :)

@voxpelli
Copy link
Collaborator

That’s weird and unexpected :/ Then one can’t trust those tests really

@XhmikosR
Copy link
Contributor

XhmikosR commented Oct 24, 2024 via email

@voxpelli
Copy link
Collaborator

As long as the tests still pass on Windows I’m 🙌

I mean, this was my review comment @XhmikosR

@bcomnes
Copy link
Owner Author

bcomnes commented Oct 24, 2024

As a windows user worried about compatibility, it seems like wsl would be the better solution to this and for many of the other edge cases similar to this. Is this not accessible by default for some reason? (Sorry I haven't run windows in years).

Also, as a dev time only dep, you would also need access to git, which would also seem to necessitate wsl or git bash.

My goal is to cut down on everywhere deps when there are better solutions available because I get like 50+ prs when they major version.

@voxpelli
Copy link
Collaborator

@XhmikosR That bash is the default on windows seems to be incorrect? The documentation says that this is the default:

Windows pwsh

This is the default shell used on Windows. The PowerShell Core. GitHub appends the extension .ps1 to your script name. If your self-hosted Windows runner does not have PowerShell Core installed, then PowerShell Desktop is used instead.

pwsh -command ". '{0}'".


@bcomnes If you make a config like I did here you can opt to ignore such automated PR:s for specific dependencies: https://github.com/voxpelli/renovate-config/blob/main/ignores.json


Maybe we should restore some note in the docs? I'm okay with keeping the npm scripts as they are

@voxpelli
Copy link
Collaborator

@bcomnes I wonder if it would be useful to run the CI tests in more of the shells available: https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#defaultsrunshell

@voxpelli
Copy link
Collaborator

This is what rimraf itself uses to run its tests: https://github.com/isaacs/rimraf/blob/8733d4c30078a1ae5f18bb6affe83c1eea0259b4/.github/workflows/ci.yml#L15-L18

        node-version: [20.x, 22.x]
        platform:
        - os: ubuntu-latest
          shell: bash
        - os: macos-latest
          shell: bash
        - os: windows-latest
          shell: bash
        - os: windows-latest
          shell: powershell

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