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

Fix: copy recursively from read only location. (fixes #98) #555

Closed
wants to merge 1 commit into from
Closed

Fix: copy recursively from read only location. (fixes #98) #555

wants to merge 1 commit into from

Conversation

Fransz
Copy link

@Fransz Fransz commented Nov 13, 2016

I think this fixes #98
Tested on OS X, but not on windows/linux.

A test was added.

@nfischer
Copy link
Member

@Fransz thanks so much for the PR! Can you investigate the windows failure? This may be due to chmod not working correctly on Windows. If so, feel free to skip the tests on Windows.

Can you rewrite the tests to not modify resources? Copy the files out first, then modify them, and then do a copy to test that read-only works.

Can you please add a test for an individual read-only file as well?

@Fransz
Copy link
Author

Fransz commented Nov 14, 2016

Tests are adjusted, and skipped on windows as asked by @nfischer

I'm not sure to change the file permissions on the copied directories/files.
The -p flag should be used for that really.

@nfischer
Copy link
Member

@Fransz can you also verify that the permissions are still correct after copying? This should do the trick:

assert.equal(fs.statSync('tmp/cp_cp').mode & parseInt('777', 8), parseInt('555', 8));

@nfischer nfischer self-assigned this Nov 14, 2016
@nfischer nfischer added the fix Bug/defect, or a fix for such a problem label Nov 14, 2016
@nfischer nfischer added this to the v0.7.x milestone Nov 14, 2016
@Fransz
Copy link
Author

Fransz commented Nov 15, 2016

I've added assertions for the permissions being correct after copying as @nfischer requested.
This doesn't pass travis however;

The test fails on a single job (not the complete travis build);
Repeating the build fails again, but now on a different job (i.e. different node version);

I've tried several times. The only thing in common seems to be that all failures happend on OS X jobs.

I'm new to travis, so i really do not know how to fix this.

@nfischer
Copy link
Member

@Fransz I'll look into Travis. Sounds like flakiness, either in the test or in Travis. I'll ping this if there's any action we need from you. Thanks for pointing it out!

@nfischer
Copy link
Member

To me, this doesn't look like a bug that is due to Travis. I can't reproduce it locally though, so I restarted the whole build. If it passes with no failures, we can merge now and fix it the next time the failure arises. Otherwise we'll investigate further.

I may mark the test with a TODO to examine the flakes later.

@nfischer
Copy link
Member

Just restarted the test again. Since it seems like the test keeps flaking, @Fransz would you mind redoing the tests after #565 is merged? Using a real framework may help with test reliability.

@Fransz
Copy link
Author

Fransz commented Nov 22, 2016

@nfischer, i rewrote the tests in ava.
Unfortunately this doesn't seem to help. Tests keeps failing in an unpredicatable way.

@nfischer
Copy link
Member

@Fransz thanks for doing the refactor. It looks like other tests in cp are flaking now. Those flakes are probably due to our refactor to ava.

I'm going to see if I can resolve the general flakiness, and then I'll try to merge PR in.

@nfischer
Copy link
Member

nfischer commented Jun 7, 2017

Bumping this back to v0.8, since we're focusing efforts there.

@nfischer nfischer modified the milestones: v0.8.0, v0.7.x Jun 7, 2017
@nfischer nfischer modified the milestones: v0.8.0, v0.9.0 Oct 20, 2017
@nfischer
Copy link
Member

Bumping to v0.9 milestone

@nfischer
Copy link
Member

nfischer commented Jul 4, 2018

Unfortunately, this has sat unmerged for a long time. Looking at the code, this doesn't seem to be too hard to cherry pick to a new branch. I'll try this out.

@nfischer
Copy link
Member

nfischer commented Jul 4, 2018

Close in favor of #870. Thanks for this fix!

@nfischer nfischer closed this Jul 4, 2018
nfischer added a commit that referenced this pull request Jul 10, 2018
This is a redo of PR #555.

This rebases, cleans up a test, and fixes a bug (the original PR uses `fs.chown()` instead of `fs.chownSync()`).

Fixes #98
@nfischer nfischer modified the milestones: v0.9.0, v0.8.x Jul 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug/defect, or a fix for such a problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cp does not recursively copy from readonly location
2 participants