AdaCore / e3-core

Core framework for developing portable automated build systems
26 stars 36 forks source link

sync_tree sometimes fails to copy the read-only attribute on Windows #680

Open sebastianpoeplau opened 7 months ago

sebastianpoeplau commented 7 months ago

In some cases, e3.fs.sync_tree fails to synchronize the Windows read-only attribute on files. You can reproduce the problem using the following script:

from e3.fs import sync_tree
from e3.os.fs import chmod

chmod("a-wx", "in/foo.txt")
sync_tree("in", "out", delete=True)

(It doesn't seem to matter whether delete is True or False.)

This is how I used the script:

poeplau@PAWNEE ~/code/sync_tree
$ mkdir in out

poeplau@PAWNEE ~/code/sync_tree
$ touch {in,out}/foo.txt

poeplau@PAWNEE ~/code/sync_tree
$ ll in out
in:
total 0
drwxrwxr-x+ 1 poeplau None 0 Feb 13 12:14 .
drwxrwxr-x+ 1 poeplau None 0 Feb 13 12:14 ..
-rw-rw-r--+ 1 poeplau None 0 Feb 13 12:14 foo.txt

out:
total 0
drwxrwxr-x+ 1 poeplau None 0 Feb 13 12:14 .
drwxrwxr-x+ 1 poeplau None 0 Feb 13 12:14 ..
-rw-rw-r--+ 1 poeplau None 0 Feb 13 12:14 foo.txt

poeplau@PAWNEE ~/code/sync_tree
$ python sync_tree_test.py 

poeplau@PAWNEE ~/code/sync_tree
$ ll in out
in:
total 0
drwxrwxr-x+ 1 poeplau None 0 Feb 13 12:14 .
drwxrwxr-x+ 1 poeplau None 0 Feb 13 12:14 ..
-r--r--r--+ 1 poeplau None 0 Feb 13 12:14 foo.txt

out:
total 0
drwxrwxr-x+ 1 poeplau None 0 Feb 13 12:14 .
drwxrwxr-x+ 1 poeplau None 0 Feb 13 12:14 ..
-rw-rw-r--+ 1 poeplau None 0 Feb 13 12:14 foo.txt

Note how out/foo.txt is still writable after the call to sync_tree. Windows Explorer confirms that the read-only attribute isn't set.

sebastianpoeplau commented 7 months ago

Here's the output of attrib before and after running the script, in both directories.

# Before
# in
A            C:\home\poeplau\code\sync_tree\in\foo.txt
# out
A            C:\home\poeplau\code\sync_tree\out\foo.txt

# After
# in
A    R       C:\home\poeplau\code\sync_tree\in\foo.txt
# out
A            C:\home\poeplau\code\sync_tree\out\foo.txt
leocardao commented 2 months ago

Hi @sebastianpoeplau,

I don't think it's a bug as such and I'm not sure it's something we want to fix.

In fact you created your out/foo.txt file before the sync_tree. When you do a touch on windows, it automatically gets u+rw as the mode (the rest is read-only). What's more, your 2 files are totally identical in content and even in timestamp (well, I get, most of the time, identical timestamps in my tests, so I assume you do too).

sync_tree does a synchronization between 2 folders. But if you place a file in the destination folder yourself and the only thing you change is the rights ... How do the code know if it's something the dev wanted or if it's a mistake? In fact, you're literally in a case where sync_tree just doesn't do anything, it doesn't touch a single file and therefore never changes the rights.

The proof is if you test like this:

rm -rf in out
mkdir -p  in out

touch {in,out}/foo.txt

ls -la in out

touch in/foo.txt

python testsuite.py

ls -la in out

(I add an intermediate touch in/foo.txt to update the file timestamps and testsuite.py is exactly the python script you provide on this issue)

By doing this, the file in/foo.txt will have different timestamps than the one in out/foo.txt, so sync_tree will update the file out/foo.txt which will then have the right rights.

I'm going to check with the team to see if this is the way we want to do things. I'll be back as soon as I have some information to give you.

sebastianpoeplau commented 2 months ago

Hi @leocardao,

But if you place a file in the destination folder yourself and the only thing you change is the rights ... How do the code know if it's something the dev wanted or if it's a mistake?

How does it know when the content changes that I haven't made a mistake? :upside_down_face:

By doing this, the file in/foo.txt will have different timestamps than the one in out/foo.txt, so sync_tree will update the file out/foo.txt which will then have the right rights.

I'm not sure whether this just is a problem with my reproducer :thinking: For context, the real issue where this came up is the AdaCore build of the GNAT-LLVM runtime: The runtime's Makefile executes chmod a-wx *.ali on the runtime to make the .ali files read-only; this is critical because gprclean uses the read-only flag on Windows to determine whether object files should be cleaned up. But after a sync_tree to the installation directory the files were writable again, resulting in gprclean deleting runtime objects. You can reproduce the problem by removing the calls to protect_ali_files (defined in gnat-llvm-core.anod).