build_log: make sure to carry the old uid and gid #1362
Conversation
marcelhollerbach
force-pushed
the
master
branch
from
7386adf
to
405632a
Compare
see ninja-build#1302 as a normal workflow you do things like "ninja all && sudo ninja install" for the case that the install target executes a build target then ninja_log will be writte ntwo which will result in its permissions to be root / root, which is a problem since you cannot execute the workflow from above again.
this file does the same, and should also maintain the permissions
marcelhollerbach
force-pushed
the
master
branch
from
405632a
to
052e879
Compare
|
Okay its resolved, i saw that the problem is also in deps_log, so i added the same there! |
|
How portable is this? (What about Windows?) |
it is now a function called ReplaceContent, since it will replace the content of a file, while carrying the ownership of the file.
|
I have 2 patches that are making them portable, i just can compile them, but cannot run them (dont have a windows maschine arround ... ) I hope i can get tomorrow a running windows maschine and run it on there ... |
this will just delete the path to replace. The ownership issue like we have it on windows is way
|
I made it now work on windows (took a while to dive into theire api). Works now on windows and linux! Any other comments? Or can it proceed ? |
|
Is there anything blocking this PR to get accepted? |
| } | ||
| } | ||
| #else | ||
| if (!CopyFile(old_path.c_str(), path.c_str(), false)) |
There was a problem hiding this comment.
This is the same behavior as the unlink/rename you're replacing, right? can we just keep using unlink/replace then?
And then that part could be shared still and it'd look like
#if !defined(_WIN32)
stat()
#endif
unlink()
rename()
#if !defined(_WIN32)
chown()
#endfi
There was a problem hiding this comment.
Well, as far as I remember CopyFile keeps the owner of the new file as long as it exists. And only copies the content, i cannot find any source anymore saying that. However, when doing only unlink() rename() the same issue we have on linux hits on windows ...
There was a problem hiding this comment.
Ah, thanks. Sounds subtle. I could imagine someone coming along and "simplifying" this code to what I just proposed some time later. I wonder if there's something we could do to stop them since using CopyFile() is important here. Do you think it's possible to write an automated test for that? Maybe create a file with non-standard permissions, call CopyFile() and check the new file has the non-standard permissions? Ideally in a way that the test fails if someone does the change I proposed here.
| found_uid_gid = false; | ||
| } | ||
|
|
||
| if (unlink(path.c_str()) < 0) { |
There was a problem hiding this comment.
Per #1572, this should probably ignore ENOENT errors.
|
This is replaced with #1588, i cannot update this one here anymore |
see #1302 as a normal workflow you do things like "ninja all && sudo
ninja install" for the case that the install target executes a build
target then ninja_log will be writte ntwo which will result in its
permissions to be root / root, which is a problem since you cannot
execute the workflow from above again.