cmd-shim icon indicating copy to clipboard operation
cmd-shim copied to clipboard

Published 20 hours ago • verified publisher icon npm

Drop env's -S flag. (#54)

Open yeldiRium opened this issue 3 years ago • 8 comments

I want to offer an implementation that achieves what I have detailled in #54.

I've added tests according to the test setup as I have found it. Please tell me if I am missing quality control somewhere.

References

  • #54

yeldiRium avatar Jul 27 '21 09:07 yeldiRium

I'm having the issue described in #54 as well. I don't see any side effects or downsides, any chance to get this merged?

nathanlepori avatar Aug 25 '21 07:08 nathanlepori

I'll ping the most active contributors in hopes that they have maintainer rights and take a look at this. @isaacs @ForbesLindesay

yeldiRium avatar Oct 08 '21 07:10 yeldiRium

Looks fine to me. We should probably ignore any --prefixed arguments to env in the shebang, tbh.

(Note: I'm no longer a committer on this project. cc: @darcyclarke to have someone on the npm cli team take a look.)

isaacs avatar Nov 29 '21 16:11 isaacs

I hope this can be fixed in 2022! :)

huan avatar Feb 11 '22 09:02 huan

@ruyadorno / @nlf can either of you take a look at this if/when you may get some time over the next week or so.

darcyclarke avatar Feb 16 '22 05:02 darcyclarke

ping @ruyadorno @nlf

huan avatar Feb 28 '22 02:02 huan

+1 for this being merged. See my comment https://github.com/npm/cmd-shim/issues/54#issuecomment-1211054310 for why the current state makes cross-platform "bin" impossible.

We should probably ignore any --prefixed arguments to env in the shebang, tbh.

This is less clear to me. For example, -u should unset environment variables. Supporting these correctly would require more work, so it seems better to leave them in and crash, to indicate lack of support.

Luckily, the uses of -S require that it must be the very first argument, so this PR supports what we need for crossplatform scripts.

A small comment: I think (?:\/usr\/bin\/env)?\s* should be (?:\/usr\/bin\/env)?\s+ to prevent /usr/bin/envfoo being interpreted as an env shebang line. This is a bug in the current regexp, not directly relevant to this PR, though.

@yeldiRium Could you maybe resolve the conflicts to increase chance of merging?

edemaine avatar Aug 10 '22 18:08 edemaine

Hi @edemaine,

thanks for the feedback. I updated the branch.

I didn't follow the project in the meantime, so I'm not sure if my changes are still valid. What stands out is that there are ESLint errors that occur outside of my changes.

yeldiRium avatar Aug 12 '22 08:08 yeldiRium

this pull request got a bit stale (our fault! sorry about that!) so i went ahead and rebuilt it as #96 and am closing this one

thank you! for the contribution, it's hugely appreciated and i'm really sorry this didn't go out much sooner!

nlf avatar Dec 13 '22 22:12 nlf