Made FileUtil to copy POSIX permissions #4654

lkishalmi · 2022-09-19T06:15:53Z

Well as a DevOps engineer, I copy scripts and other executable inside the IDE. They lose the executable flag. That bothers me for years. Here is how I try to fix that.

Also added FileObject Path conversion methods, to make the life easier.

sdedic · 2022-09-19T10:07:27Z

Two questions:

given that FileUtil.copy did not copy the attributes or is not specified as 'copied by stream content' ... isn't that an incompatible API change ?
what about the AclFileAttributesView - this is useful on Windows. Except for file owner, ACLs may be (usually) set by a regular user.

lkishalmi · 2022-09-19T14:58:08Z

Well that API is ancient, from those times, when the Executable permissions could not be set by Java (pre-Java 6) without some magic.

As a DevOps Engineer, I'm working with a lot of scripts recently, so I thought I fix my issue when copy them in the IDE. That's all. I do not know how important Windows ACL for other people, well if that's really missed, contributions are welcome!

mbien

looks good to me - left a comment but feel free to ignore it.

mbien · 2022-09-22T05:04:21Z

platform/openide.filesystems/src/org/openide/filesystems/FileUtil.java

            copy(bufIn, bufOut);
+            copyPosixPerms(source, dest);
            copyAttributes(source, dest);


just a thought:
Files.copy(...) would copy posix permissions too (there is also a COPY_ATTRIBUTES option). Alternative impl would check if its an actual file first, if it is, it would use Files.copy() with paths, otherwise use streams.

Maybe its worth thinking about refreshing this code if it isn't too much work? Otherwise we end up reimplementing everything what Files.copy already does.

platform/openide.filesystems/test/unit/src/org/openide/filesystems/FileUtilTest.java

platform/openide.filesystems/apichanges.xml

sdedic · 2022-09-22T13:51:04Z

platform/openide.filesystems/apichanges.xml

+            <author login="lkishalmi"/>
+            <compatibility addition="yes" semantic="compatible"/>
+            <description>
+                <a href="@TOP@/org/openide/filesystems/FileUtil.html#">FileUtil.copy</a> now preserves POSIX permissions if possible during file copy.


missing anchor to the FileUtil.copy chapter ?

jtulach

I always dreamed about 1:1 mapping between Path and FileObject. They could delegate to each other. Always. E.g. the new methods shall never return null.

neilcsmith-net · 2022-10-18T09:50:56Z

@lkishalmi still a few things to address here. I also agree with @mbien and @jtulach about 1:1 mapping behaviour with Path and Files::copy COPY_ATTRIBUTES. Not sure whether at least minimal things can be addressed and merged for NB16 or we should push all changes to NB17?

lkishalmi · 2022-10-18T16:16:24Z

I hope I can make those minimal changes needed. The day is just started for me. If I can't make that happen, you are right, I'll move this to NB17.

neilcsmith-net · 2023-01-05T11:08:29Z

@lkishalmi any thoughts on progressing this one? It's getting close to missing the NB17 cut too.

lkishalmi · 2023-01-13T03:49:32Z

I always dreamed about 1:1 mapping between Path and FileObject. They could delegate to each other. Always. E.g. the new methods shall never return null.

Well, this PR won't make that dream true. Probably one day when someone would have a lot of time. AFAIK we need to provide a few FileSystem implementation to be able to use Path in all cases which are covered by FileObject.

As of returning null is in sync with the current implementation. Returning Optional would be an outlier in the API.

lkishalmi · 2023-01-13T05:57:07Z

Unfortunately Files.copy made other tests fail in masterfs and filesystems module. I'll revert to the previous implementation.

lkishalmi · 2023-01-13T05:57:58Z

BTW. This PR marks all checks passed as it was missing the Platform label.

sdedic · 2023-01-13T16:06:54Z

platform/openide.filesystems/apichanges.xml

+        <change id="fileutil.niofilepath">
+            <api name="filesystems"/>
+            <summary>FileUtil can convert FileObject to/from java.nio.file.Path.</summary>
+            <version major="9" minor="30"/>


9.32 (master has currently 9.31). We should probably bump version for each api change to allow proper autoupdates from the daily builds for those bleeding-edge testers.

sdedic · 2023-01-13T16:07:49Z

platform/openide.filesystems/src/org/openide/filesystems/FileUtil.java

+     * This is the inverse operation of {@link #toFileObject}.
+     * @param fo FileObject whose corresponding Path will be looked for
+     * @return java.nio.file.Path or null if no corresponding File exists.
+     * @since 9.30


9.31 if apichanges is corrected.

sdedic · 2023-01-13T16:08:17Z

platform/openide.filesystems/src/org/openide/filesystems/FileUtil.java

@@ -567,6 +569,7 @@ static FileObject copyFileImpl(FileObject source, FileObject destFolder, String
            }

            copy(bufIn, bufOut);
+            copyPosixPerms(source, dest);


Please make a mote in method's Javadoc about behaviour enhancement since 9.31(2), people can stop writing attribute preserving code, if they read javadoc :)

lkishalmi added this to the NB16 milestone Sep 19, 2022

lkishalmi requested review from jtulach and sdedic September 19, 2022 06:15

lkishalmi force-pushed the fileutil-copy-posix-perms branch from 3b49c26 to 218e6c6 Compare September 20, 2022 00:02

lkishalmi requested review from mbien and neilcsmith-net September 22, 2022 03:22

mbien approved these changes Sep 22, 2022

View reviewed changes

sdedic reviewed Sep 22, 2022

View reviewed changes

jtulach reviewed Oct 4, 2022

View reviewed changes

lkishalmi modified the milestones: NB16, NB17 Oct 19, 2022

lkishalmi force-pushed the fileutil-copy-posix-perms branch from 218e6c6 to ba03950 Compare January 13, 2023 04:18

lkishalmi added the Platform [ci] enable platform tests (platform/*) label Jan 13, 2023

lkishalmi force-pushed the fileutil-copy-posix-perms branch from ba03950 to 8a88662 Compare January 13, 2023 06:13

lkishalmi requested review from mbien, sdedic and jtulach January 13, 2023 15:36

mbien approved these changes Jan 13, 2023

View reviewed changes

sdedic reviewed Jan 13, 2023

View reviewed changes

Made FileUtil to copy POSIX permissions

47676c2

lkishalmi force-pushed the fileutil-copy-posix-perms branch from 8a88662 to 47676c2 Compare January 13, 2023 22:55

lkishalmi merged commit b0d67bc into apache:master Jan 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made FileUtil to copy POSIX permissions #4654

Made FileUtil to copy POSIX permissions #4654

lkishalmi commented Sep 19, 2022

sdedic commented Sep 19, 2022

lkishalmi commented Sep 19, 2022

mbien left a comment

mbien Sep 22, 2022

sdedic Sep 22, 2022

jtulach left a comment

neilcsmith-net commented Oct 18, 2022

lkishalmi commented Oct 18, 2022

neilcsmith-net commented Jan 5, 2023

lkishalmi commented Jan 13, 2023

lkishalmi commented Jan 13, 2023

lkishalmi commented Jan 13, 2023

sdedic Jan 13, 2023 •

edited

sdedic Jan 13, 2023

sdedic Jan 13, 2023

Made FileUtil to copy POSIX permissions #4654

Made FileUtil to copy POSIX permissions #4654

Conversation

lkishalmi commented Sep 19, 2022

sdedic commented Sep 19, 2022

lkishalmi commented Sep 19, 2022

mbien left a comment

Choose a reason for hiding this comment

mbien Sep 22, 2022

Choose a reason for hiding this comment

sdedic Sep 22, 2022

Choose a reason for hiding this comment

jtulach left a comment

Choose a reason for hiding this comment

neilcsmith-net commented Oct 18, 2022

lkishalmi commented Oct 18, 2022

neilcsmith-net commented Jan 5, 2023

lkishalmi commented Jan 13, 2023

lkishalmi commented Jan 13, 2023

lkishalmi commented Jan 13, 2023

sdedic Jan 13, 2023 • edited

Choose a reason for hiding this comment

sdedic Jan 13, 2023

Choose a reason for hiding this comment

sdedic Jan 13, 2023

Choose a reason for hiding this comment

sdedic Jan 13, 2023 •

edited