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.

Sign up for GitHub

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

Jump to bottom

fix(write_source_files): fix nested directories #65

Merged
merged 14 commits into from
Apr 6, 2022
Merged

fix(write_source_files): fix nested directories #65

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ae9843a
fix(write_source_files): fix nested directories
jbedard Apr 4, 2022
2bb9fd6
add chmod back with only +w
jbedard Apr 6, 2022
09821fa
chmod recursively
jbedard Apr 6, 2022
6cd2590
also remove full dir in case files no longer exist
jbedard Apr 6, 2022
e1ff6c4
follow instead of copy symlinks when copying
jbedard Apr 6, 2022
4ff4eb9
fixo
jbedard Apr 6, 2022
c9e80dc
remove like copy
jbedard Apr 6, 2022
c1276e2
make subdir_test file readonly
jbedard Apr 6, 2022
d3ad08b
BUILD comments
jbedard Apr 6, 2022
97aabaf
update test to also have non-writeable directories
jbedard Apr 6, 2022
e6fc9d0
test symlinks - remove symlink fix
jbedard Apr 6, 2022
057a7ca
add copy symlink fix back
jbedard Apr 6, 2022
d10b838
give user+group write permission
jbedard Apr 6, 2022
ed302dd
fix write_source_files_symlinks.sh exit-on-error
jbedard Apr 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions e2e/write_source_files_subdir_multiple_runs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#!/bin/bash

set -e

bazel run //lib/tests/write_source_files:write_subdir
[ -e lib/tests/write_source_files/subdir_test/a/b/c/test.txt ]

bazel run //lib/tests/write_source_files:write_subdir
[ -e lib/tests/write_source_files/subdir_test/a/b/c/test.txt ]
7 changes: 4 additions & 3 deletions lib/private/write_source_file.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,11 +164,12 @@ echo "Copying $in to $out in $PWD"

if [[ -f "$in" ]]; then
cp -f "$in" "$out"
chmod 664 "$out"
chmod +w "$out"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So these only add write permission for the current user. Should it be for all? Seems like bazel normally does -r-xr-xr-x (makes all 3 the same). Should we do similar and add w for all?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User and group is probably safe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

else
rm -Rf "$out"/*
mkdir -p "$out"
cp -fR "$in"/* "$out"
chmod 664 "$out"/*
cp -fRL "$in"/* "$out"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think capital -R is or will be deprecated on mac? I recall @gregmagolan had a PR out about that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the L for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L makes it follow symlinks instead of copy them.

If bazel creates a directory with symlinks to other locations we don't want to just copy the links but copy the content they point to. Otherwise we end up with symlinks into the sandbox copied into the source dir...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rules_nodejs pkg_web seems to create these symlinks which is where we saw the problem. I think pkg_web creates a directory structure for the package but then the files within those directories are symlinks to the original source of files being "packaged". Basically avoid copying files all over the place and use symlinks as much as possible.

When writing that package into the source dir we want to copy the real files and not the symlinks...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-R is the correct one with consistent meaning on both platforms. It is the lowercase -r that has a different meaning on macos vs linux.

chmod -R +w "$out"/*
fi
""".format(in_path = in_path, out_path = out_path))

Expand Down
3 changes: 2 additions & 1 deletion lib/tests/write_source_files/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
dist.js
dist.js
subdir_test
20 changes: 20 additions & 0 deletions lib/tests/write_source_files/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,28 @@ genrule(
)

# ibazel run //lib/tests/write_source_files:write_dist
# See e2e/write_source_files_gitignored
write_source_files(
name = "write_dist",
diff_test = False,
files = {"dist.js": ":dist"},
)

# Generate a readonly file in nested readonly directories
genrule(
name = "subdir",
outs = ["subdir_test"],
cmd = ";".join([
"mkdir -p $@/a/b/c",
"echo 'test' > $@/a/b/c/test.txt",
"chmod -R -w $@/*"
]),
)

# Write nested subdirectories to source
# See e2e/write_source_files_subdir_multiple_runs
write_source_files(
name = "write_subdir",
diff_test = False,
files = {"subdir_test": ":subdir_test"},
)