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

replace bash subcommands with separate shell spawns #69

Closed
wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Oct 14, 2019

As per discussion in #66

I need to think about this a bit more, there's likely going to be overhead for non-windows users because this replaces all Bash subcommands with separate spawn(shell) executions and piecing the output together to form the final git log command.

It also leaves this repo in a 1/2 promisified state which is a bit awkward.

But, let's see what CI does.

Closes #63, fixes #54

@rvagg
Copy link
Member Author

rvagg commented Oct 14, 2019

👍 passing on windows, though I've removed 8.x in the latest commit, stream.pipeline() is a 10.x feature and util.promisify() is improved in 10.x too.

@XhmikosR
Copy link
Contributor

Remember that there's a case where things worked on Windows CI but still failed on vanilla Windows cmd :)

Please ping me when you think this is ready so that I try it on my Windows VM.

@XhmikosR
Copy link
Contributor

XhmikosR commented Oct 15, 2019

@rvagg just tested this on my Windows VM, with /origin/xmr-pack-gitexec-up merged in this branch and seems to work fine.

C:\Users\xmr\Desktop\changelog-maker>ver

Microsoft Windows [Version 10.0.18362.418]

C:\Users\xmr\Desktop\changelog-maker>node -v && npm -v
v12.12.0
6.11.3

C:\Users\xmr\Desktop\changelog-maker>npm t

> [email protected] test C:\Users\xmr\Desktop\changelog-maker
> npm run lint && npm run unit


> [email protected] lint C:\Users\xmr\Desktop\changelog-maker
> standard


> [email protected] unit C:\Users\xmr\Desktop\changelog-maker
> tape test.js

TAP version 13
# test basic commit block
ok 1 should be equal
# test filter-release
ok 2 should be equal
# test simple
ok 3 should be equal
# test group, semver labels, PR-URL
ok 4 should be equal
# test simple group, semver labels, PR-URL
ok 5 should be equal
# test blank commit-url
ok 6 should be equal
# test blank commit-url
ok 7 should be equal

1..7
# tests 7
# pass  7

# ok

BTW, so that we are safe maybe we should close #66 and you rebase this branch to include the patch instead?

EDIT: NVM you updated to 2.0.1 here too.

package.json Outdated Show resolved Hide resolved
@rvagg
Copy link
Member Author

rvagg commented Oct 16, 2019

I can measure a very small performance impact by this. In the order of 10-20ms, and it's going to depend a bit on I/O speed.

I think that having this work on Windows is probably worth the price. Unwinding some of the complexity of the git commands is also going to be a bonus here I think--the way it's currently structured makes it quite unfriendly for contributors I think. I would like some other opinions on that though.

This change will be semver-major because we're dropping support for older Node. I wouldn't mind getting at least a new ghauth() in before a release too, that should mostly take care of the TODO in here but will be a separate PR because it needs a refactor of collect-commit-labels.js.

@XhmikosR
Copy link
Contributor

XhmikosR commented Oct 16, 2019

Yup, I noticed a small perf impact too. But I agree with your points.

@rvagg rvagg mentioned this pull request Mar 16, 2020
@rvagg rvagg closed this Jun 24, 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

Successfully merging this pull request may close these issues.

Make it work on Windows Some warnings
2 participants