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

Ensure that `.bin` files maintain executable bits in Windows bundles. #8503

Merged
merged 1 commit into from Mar 21, 2017

Conversation

Projects
None yet
2 participants
@abernix
Member

abernix commented Mar 20, 2017

#Windows has no concept of the executable bit so it is not applied by the fstream Reader when building the tarball which is used in both meteor build and meteor deploy. For Windows users, this causes important scripts (such as node-pre-gyp) to not be executable when the bundles are deployed to Unix platforms (such as Galaxy).

To avoid giving every file executable bits, this applies an executable bit to the file only if it has read permission (something Windows is aware of) and if it is in a location that Node bin links are typically placed, the /node_modules/.bin/ directories.

Additionally, to apply a slightly more conservative approach to the existing logic (which applied executable bits to all directories), this changes that logic to only apply the executable bit to readable directories, thus maintaining some degree of permissions control when building from a Windows system. This is in line with other projects linked in the comments and issues below, such as node-pre-gyp and other projects linked in the node-tar issue.

A symptom of this might be an error message such as:

> node-pre-gyp install --fallback-to-build

sh: 1: node-pre-gyp: Permission denied

@abernix abernix requested a review from benjamn Mar 20, 2017

@@ -926,8 +940,13 @@ files.createTarGzStream = function (dirPath, options) {
// setting it in an 'entry' handler is the same strategy that npm
// does, so we do that here too.
if (entry.type === "Directory") {
entry.mode = (entry.mode || entry.props.mode) | 0o500;
entry.props.mode = entry.mode;
entry.props.mode = addExecBitWhenReadBitPresent(entry.props.mode);

This comment has been minimized.

@benjamn

benjamn Mar 21, 2017

Member

Why is the | 0o500 no longer necessary? In other words, why not something like this:

entry.props.mode = addExecBitWhenReadBitPresent(
  (entry.mode || entry.props.mode) | 0o500
);

This comment has been minimized.

@abernix

abernix Mar 21, 2017

Member

First, I should point out that entry.mode is always undefined and never set, or used – maybe it was used in a previous version of fstream, but I can't find any (we use our own fork), and this behavior matches the published fstream behavior. The second parameter to the filter callback is actually props, so this would make sense in that scenario but this code doesn't do that, instead utilizing only the first argument which includes props as an attribute on entry.

For directories, I'd say that the | 0o500 (e.g. u+rx) was never necessary and that something less would have accomplished the goal. I'm happy to change that back, but my thinking was that it might be preferable to maintain some sense of permissions on Windows. Essentially, if a directory was encountered that had the readable bit missing, we shouldn't simply add that read bit and add the executable bit. Presumably there would be some reason that read permission (something Windows does understand) was not set. My train of thought was strongly influenced by that of the other PRs marked as closed and merged in the node-tar issue, for example, mapbox/node-pre-gyp@7a28f4b and Microsoft/types-publisher@be20819#diff-db71419dab88a2797059646c6a31a328R42.

@benjamn benjamn changed the base branch from devel to release-1.4.3.x Mar 21, 2017

Ensure that `.bin` files maintain executable bits in Windows bundles.
Windows has no concept of the executable bit so it is not applied by the
`fstream` `Reader` when building the tarball which is used in both
`meteor build` and `meteor deploy`.  For Windows users, this causes
important scripts (such as `node-pre-gyp`) to not be executable when
the bundles are deployed to Unix platforms (such as Galaxy).

To avoid giving every file executable bits, this applies an executable
bit to the file only if it has read permission (something Windows _is_
aware of) and if it is in a location that Node bin links are typically
placed, the `/node_modules/.bin/` directories.

@benjamn benjamn force-pushed the abernix:abernix/fix-windows-bin-link-perms branch from 647bd0d to ac55072 Mar 21, 2017

@benjamn benjamn merged commit 17a786e into meteor:release-1.4.3.x Mar 21, 2017

1 of 3 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
CLA Author has signed the Meteor CLA.
Details

@benjamn benjamn added this to the Release 1.4.3.x milestone Mar 21, 2017

@abernix abernix modified the milestones: Release 1.4.3.x, Release 1.4.3.3 Mar 21, 2017

abernix added a commit that referenced this pull request Mar 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment