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 executable arguments to spawn in win32 #611

Merged
merged 2 commits into from Jun 24, 2019
Merged

add executable arguments to spawn in win32 #611

merged 2 commits into from Jun 24, 2019

Conversation

hxsf
Copy link
Contributor

@hxsf hxsf commented Mar 16, 2017

In win32, commander just spawn the subcommander in this package's bin scripts.
so, just add the process.execArgv into args is ok.

In win32, commander just spawn the subcommander in this package's `bin` scripts.
so, just add the `process.execArgv` into `args` is ok.
@shadowspawn
Copy link
Collaborator

@hxsf Can you give an example of what this is fixing, or link to some issues that this would fix?

I think because the current code is calling spawn(process.execPath that it is probably appropriate to add process.execArgv as proposed. This matches the isExplicitJS code in the non-windows code branch.

@hxsf
Copy link
Contributor Author

hxsf commented Apr 23, 2019

the example project: https://gist.github.com/hxsf/80a51ded2817ba2cb8c36f55e9ec77d4

there is a '--harmony' args in both two js file

if run demo-list will get

[demo-list] [ '--harmony' ]

if run demo list (the sub command mode)
will get

[demo] [ '--harmony' ]
[demo-list] []

we can see, in the sub command mode, the execArgv lost

so I want to add them back.

but, it is just add the execArgs from main to sub.
I think we need to parse the first line of sub command to get the execArgv we need

@shadowspawn shadowspawn self-assigned this Apr 23, 2019
@shadowspawn
Copy link
Collaborator

Great, thanks @hxsf

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

Light testing with cmd, PowerShell, and git bash.

Because this changes node flags passed to subcommand (if node flags present), I think we will need to wait for a major version to release.

@shadowspawn shadowspawn removed their assignment Apr 30, 2019
@shadowspawn
Copy link
Collaborator

(I think the Travis failures are because of the old .travis.yml file at the time of the Pull Request?)

@shadowspawn
Copy link
Collaborator

so I want to add them back.

but, it is just add the execArgs from main to sub.
I think we need to parse the first line of sub command to get the execArgv we need

Adding back the execArgs is what we do on non-Windows, rather than parse first line (implicitly or explicitly), so this is consistent.

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

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

Thank you!

@abetomo abetomo merged commit 1979261 into tj:master Jun 24, 2019
@shadowspawn
Copy link
Collaborator

shadowspawn commented Jun 24, 2019

@abetomo
I am merging into release/3.0.0 rather than master so that there are not breaking changes on master until we are ready to release (in case people pulling from git).

I have not tried edited the Pull Request to change the destination branch (not even sure if we can do that), as I was worried might lose review comments if I did that.

So I have been manually merging Pull Requests into release/3.0.0 and marking them as Pending Release once merged. (This means doing merges very manually, and not using Merge button in GitHub.)

@abetomo
Copy link
Collaborator

abetomo commented Jun 24, 2019

Oh, I'm sorry!

@abetomo
Copy link
Collaborator

abetomo commented Jun 24, 2019

Revert "add executable arguments to spawn in win32" #981

@shadowspawn
Copy link
Collaborator

Something for future, we might add a develop branch and ask people to submit Pull Requests against that to make the Merge button safer! It does mean an extra step for releases as there is a merge from develop to master, but everything can land at once with the version number change and code changes and changelog.

Have you used master+develop in other projects? (This naming is used in GitFlow pattern.)

@abetomo
Copy link
Collaborator

abetomo commented Jun 24, 2019

There was also a project using develop.
I think it's good.

@shadowspawn
Copy link
Collaborator

shadowspawn commented Jun 24, 2019

  1. Thanks for review @abetomo ! 😄 I will manually merge from this Pull Request into release/3.0.0.

  2. I will add a develop branch and update the CONTRIBUTING guidelines.

  3. I did a lot in Commander in the weekend, you would have got a lot of emails! FYI: I added some milestone comments at the end of this issue before closing it: Commander.js v3.0 #666

  4. Sorry about the email noise @hxsf. 🙂

shadowspawn added a commit that referenced this pull request Jun 24, 2019
shadowspawn added a commit that referenced this pull request Jun 24, 2019
@shadowspawn
Copy link
Collaborator

Merged master down to release/3.0.0 and reapplied this change after the merge+revert. Will be included in v3 release.

Thank you for your contributions.

@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Jun 26, 2019
@shadowspawn
Copy link
Collaborator

Available now as a prerelease. See #1001

@shadowspawn
Copy link
Collaborator

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