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

Reliance on bash rm command breaks Windows #452

Closed
john-tipper opened this issue Jan 4, 2021 · 6 comments
Closed

Reliance on bash rm command breaks Windows #452

john-tipper opened this issue Jan 4, 2021 · 6 comments

Comments

@john-tipper
Copy link
Contributor

The use of rm -fr lib/ as the first step within the test script of generated projects breaks on Windows. Whilst it is possible to work around this with some hackery (e.g. defining a different test script and using that), it would be nice to strip out the OS dependency.

I think the issue is here:

this.testTask.prependExec(`rm -fr ${this.libdir}/`);

We could do something like use https://github.com/shelljs/shx as a way of using portable Shell commands (I'm sure others may think of other options). Are there any objections to doing this please? If so, does the library get declared as a dependency of Projen, or a peer dependency?

I'm happy to submit a PR, but I want to make sure others are happy with this approach before I do.

@Chriscbr
Copy link
Contributor

Chriscbr commented Jan 4, 2021

I'm not familiar with the portable shell command ecosystem, but the idea sounds reasonable to me.

If so, does the library get declared as a dependency of Projen, or a peer dependency?

I think shx would be added as a dev dependency because it's only used for running development scripts/tasks (I think this logic applies both for projects using projen, and for projen itself)? But I'm not totally sure.

@rpawlaszek
Copy link

I have the same issue on Windows with rm -fr. From my side the proposition is to use rimraf and have it as a global dependency for projen.

@john-tipper
Copy link
Contributor Author

Yes, the reason I went with shx in #540 was because there are other breaking commands other than rm that are used with Projen, such as mv and mkdir.

mergify bot pushed a commit that referenced this issue Feb 26, 2021
There was around 30+ unit tests failing on Windows due to:

- line ending differences between windows and posix
- path separator differences between windows and posix
- echo "foo" would include the quotes in stdout in windows
- paths not being quoted in `exec` calls, causing paths with spaces to get messed up

There are still some tests failing due for other reasons that need to be resolved in follow up pull requests, e.g.: tests that refer to $PWD or the pwd command (some potential fixes are discussed in #452).

Also snapshot tests seem to get deleted after the tests run (not entirely sure why), so I wasn't able to verify if these were getting generated with differences or not.

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@pgollucci
Copy link
Contributor

I think you need it in regular deps for when external projects happen.

mergify bot pushed a commit that referenced this issue May 16, 2021
This PR tries to address #452 by wrapping the commands 'rm', 'mkdir' and 'mv' with `shx`.
A whitelist of commands is maintained in the task runtime.

@eladb Is this what you had in mind when commenting on #540?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@github-actions
Copy link
Contributor

This issue is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon.

@github-actions github-actions bot added the Stale label Jun 30, 2021
@eladb
Copy link
Contributor

eladb commented Jun 30, 2021

I believe this is resolved by #781

@eladb eladb closed this as completed Jun 30, 2021
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

No branches or pull requests

5 participants