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

Maintain file modes when copying a directory. #1808

Closed
wants to merge 1 commit into from

Conversation

@jfhamlin
Copy link
Contributor

@jfhamlin jfhamlin commented Feb 3, 2014

A package may depend on some files in its dependencies being executable,
so builder ought to respect the modes of source files when copying into
a bundle.

A package may depend on some files in its dependencies being executable,
so builder ought to respect the modes of source files when copying into
a bundle.
@jfhamlin
Copy link
Contributor Author

@jfhamlin jfhamlin commented Feb 3, 2014

I may also alter the copy to use fs' read/write streams rather than reading the entire file into a buffer. This would address the comment, "XXX avoid reading whole file into memory."

@glasser
Copy link
Member

@glasser glasser commented Feb 13, 2014

I mean, this does look strictly better, but I want to understand the end-to-end use case where this is observable, because if we fix this and users rely on the behavior, then we want to make sure not to break it later (eg, there are multiple steps in bundling, some of them might use a different function, etc).

I guess your use case is including an executable as a server-side asset (eg the private directory of an app), that you then execute? We don't actually have an API for giving you the path to an asset but I suppose you're being resourceful and coding in what the current implementation gives you?

Can you say more about what exactly you're trying to accomplish? I'm just imagining writing a test for this fix and realizing that it seems like I'd have to do something that we don't provide an API for.

@jfhamlin
Copy link
Contributor Author

@jfhamlin jfhamlin commented Feb 13, 2014

Can you say more about what exactly you're trying to accomplish?

My particular use case is actually simpler than what you mention. I'm trying to create a package for the closure compiler, for which there already exists a node module. That module bundles a Java distribution in case there is no global Java installation (or if the global Java is outdated). The Meteor bundler is throwing away execute privileges on the module's own bundled Java.

So clearly you can hit this in a test by relying on a node module that itself relies on an executable, which seems legal as far as Meteor is concerned.

[Edit: s/write/execute/]

@glasser
Copy link
Member

@glasser glasser commented Feb 13, 2014

Ah, good point. That sounds pretty testable too. I'm currently working on merging a new test suite for our tool so I'll add a test for this (and merge the fix).

glasser added a commit that referenced this pull request Feb 14, 2014
glasser added a commit that referenced this pull request Feb 14, 2014
@glasser
Copy link
Member

@glasser glasser commented Feb 14, 2014

Thanks, merged!

If you send a PR for the streaming, we'd take a look... it's something I've meant to do for a while and I know it's pretty easy now that all of tools are fiberized; just need to make sure to get error handling right.

@glasser glasser closed this Feb 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.