search

npm / cmd-shim

The cmd-shim used in npm
ISC License
76 stars 40 forks source link

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

Closed yeldiRium closed 1 year ago

yeldiRium commented 3 years ago

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

nathanlepori commented 3 years ago

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?

yeldiRium commented 3 years ago

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

isaacs commented 3 years ago

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.)

huan commented 2 years ago

I hope this can be fixed in 2022! :)

darcyclarke commented 2 years ago

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

huan commented 2 years ago

ping @ruyadorno @nlf

edemaine commented 2 years ago

+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?

yeldiRium commented 2 years ago

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.

nlf commented 1 year ago

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!