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

#452 Fix broken Windows shell commands by running through shx. #540

Closed
wants to merge 1 commit into from
Closed

#452 Fix broken Windows shell commands by running through shx. #540

wants to merge 1 commit into from

Conversation

john-tipper
Copy link
Contributor


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

shx is added as a dev dependency in order for it to be available for use in scripts. Linux script commnds (e.g. rm -fr ...) are modified to run through shx in order to be supported on Windows.

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

I am wondering if we can auto-discover these commands in task.exec() and add shx automatically?

@john-tipper
Copy link
Contributor Author

What mechanism have you got in mind for the auto-discovery?

@eladb
Copy link
Contributor

eladb commented Feb 16, 2021

What mechanism have you got in mind for the auto-discovery?

For example we can have a list of supported commands in shx and when the runtime identifies that we are on windows, it will pass the exec command through it.

@john-tipper
Copy link
Contributor Author

john-tipper commented Feb 16, 2021

In the spirit of YAGNI, what benefit does this (OS discovery) give us versus just passing the currently broken rm, mkdir and mv commands through shx for all OS?

@john-tipper
Copy link
Contributor Author

Having given it some further thought, I don't disagree with the idea of supporting other commands per se, but I think determining whether a command matches something in a defined list of supported shx commands could be a pain. Whilst it's trivial to do a replace in the string of a matching command (e.g. rm -> shx rm), there are edge cases around what happens if a command is wrapped in quotes, for instance:

find /some/path --exec bash -c 'rm {}' \;

(Apologies for the badly formatted find command, I'm just trying to illustrate that a user might supply a command that may be difficult to parse). Also, we need to consider what happens if the user is running inside Windows but using something like a GitBash shell.

My instinct is to make this the responsibility of the person writing the command(s) to ensure OS compatibility as the user themselves requires it and just wrap the commands that are inside projen with shx. Just my 2c, though.

@eladb
Copy link
Contributor

eladb commented Mar 1, 2021

This feels too error prone and easy to forget to do. If there's a way we can encapsulate this behavior behind the task runner, I rather we take that path.

@eladb
Copy link
Contributor

eladb commented Mar 17, 2021

Closing in the meantime, feel free to open when you'd like to continue working on this. 🙏

@eladb eladb closed this Mar 17, 2021
@iwt-janbrauer
Copy link

Hm, why not run all commands passed to Task.exec through shx? If a command is not supported by shx, it will error. I would rather optimize for users of projen, in this case Windows users, rather than optimize for implementors of new projen features.

@john-tipper
Copy link
Contributor Author

In the meantime @iwt-janbrauer, here's what I use in my .projenrc.js to allow projen to run on Windows, where I overwrite the test task:

const test = project.addTask('test', {
  category: '10.test',
  description: 'Run tests',
  exec: 'shx rm -fr lib/',
});
test.spawn(project.tasks.tryFind('test:compile'));
test.exec('jest --passWithNoTests --all');
test.spawn(project.tasks.tryFind('eslint'));

mergify bot pushed a commit that referenced this pull request 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.
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.

None yet

3 participants