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

Add Sys.isreadable, Sys.iswriteable, update ispath #53320

Merged
merged 12 commits into from
Feb 20, 2024
Merged

Add Sys.isreadable, Sys.iswriteable, update ispath #53320

merged 12 commits into from
Feb 20, 2024

Conversation

fatteneder
Copy link
Member

As discussed here: #53286 (comment)

Readds the methods that were removed in #12819.

@fatteneder fatteneder requested a review from vtjnash February 13, 2024 18:42
vtjnash
vtjnash reviewed Feb 13, 2024
View reviewed changes
base/sysinfo.jl Outdated Show resolved Hide resolved
vtjnash
vtjnash reviewed Feb 13, 2024
View reviewed changes
base/sysinfo.jl Outdated Show resolved Hide resolved
@fatteneder fatteneder changed the title Add Sys.isreadable, Sys.iswriteable, Sys.exists Add Sys.isreadable, Sys.iswriteable, update ispath Feb 13, 2024
fatteneder
fatteneder commented Feb 13, 2024
View reviewed changes
base/sysinfo.jl Outdated Show resolved Hide resolved
vtjnash
vtjnash reviewed Feb 13, 2024
View reviewed changes
base/sysinfo.jl Outdated Show resolved Hide resolved
@vtjnash vtjnash added the filesystem Underlying file system and functions that use it label Feb 13, 2024
@fatteneder
Copy link
Member Author

The previous ispath seems to do more than the new one based on jl_fs_access. Need to take another look at that.

Test Failed at /cache/build/tester-amdci4-10/julialang/julia-master/julia-708a68b807/share/julia/test/file.jl:276
  Expression: length(TEMP_CLEANUP) == 1
   Evaluated: 0 == 1
Test Failed at /cache/build/tester-amdci4-10/julialang/julia-master/julia-708a68b807/share/julia/test/file.jl:285
  Expression: length(TEMP_CLEANUP) == 1
   Evaluated: 0 == 1
Test Failed at /cache/build/tester-amdci4-10/julialang/julia-master/julia-708a68b807/share/julia/test/file.jl:300
  Expression: length(TEMP_CLEANUP) == 2
   Evaluated: 0 == 2
Test Failed at /cache/build/tester-amdci4-10/julialang/julia-master/julia-708a68b807/share/julia/test/file.jl:306
  Expression: length(TEMP_CLEANUP) == 3
   Evaluated: 1 == 3
Test Failed at /cache/build/tester-amdci4-10/julialang/julia-master/julia-708a68b807/share/julia/test/file.jl:316
  Expression: TEMP_CLEANUP_MAX[] == 4
   Evaluated: 3 == 4
Test Failed at /cache/build/tester-amdci4-10/julialang/julia-master/julia-708a68b807/share/julia/test/file.jl:319
  Expression: !(ispath(t))
Test Failed at /cache/build/tester-amdci4-10/julialang/julia-master/julia-708a68b807/share/julia/test/file.jl:319
  Expression: !(ispath(t))
Test Failed at /cache/build/tester-amdci4-10/julialang/julia-master/julia-708a68b807/share/julia/test/file.jl:1278
  Expression: f("adir\0bad")
    Expected: ArgumentError
  No exception thrown

fatteneder
fatteneder commented Feb 15, 2024
View reviewed changes
base/stat.jl Outdated Show resolved Hide resolved
@fatteneder fatteneder force-pushed the fa/sys-read-write branch 2 times, most recently from f44d1a6 to 67e6a9d Compare February 16, 2024 22:29
@fatteneder
Copy link
Member Author

fatteneder commented Feb 17, 2024 •
edited
Loading

Two more things to do:

vtjnash
vtjnash reviewed Feb 17, 2024
View reviewed changes
base/stat.jl Outdated Show resolved Hide resolved
base/sysinfo.jl Outdated Show resolved Hide resolved
base/sysinfo.jl Outdated Show resolved Hide resolved
fatteneder and others added 3 commits February 17, 2024 18:49
@vtjnash vtjnash added the backport 1.11 Change should be backported to release-1.11 label Feb 17, 2024
@fatteneder
Copy link
Member Author

I don't understand the windows test failures. I collected this debug output with CI

     From worker 11:	StatStruct("C:\\Users\\julia\\AppData\\Local\\Temp\\jl_dn0CoZ\\subdir" size: 0 bytes device: 344850202 inode: 75248 mode: 0o040666 (drw-rw-rw-) nlink: 1 uid: 0 gid: 0 rdev: 0 blksz: 4096 blocks: 0 mtime:  (just now) ctime:  (just now))
      From worker 11:	StatStruct("C:\\Users\\julia\\AppData\\Local\\Temp\\jl_dn0CoZ\\subdir\\foo" size: 0 bytes device: 344850202 inode: 75265 mode: 0o100666 (-rw-rw-rw-) nlink: 1 uid: 0 gid: 0 rdev: 0 blksz: 4096 blocks: 0 mtime:  (just now) ctime:  (just now))
      From worker 11:	StatStruct("C:\\Users\\julia\\AppData\\Local\\Temp\\jl_dn0CoZ\\subdir\\foo" size: 0 bytes device: 344850202 inode: 75265 mode: 0o100666 (-rw-rw-rw-) nlink: 1 uid: 0 gid: 0 rdev: 0 blksz: 4096 blocks: 0 mtime:  (just now) ctime:  (just now))
      From worker 11:	StatStruct("C:\\Users\\julia\\AppData\\Local\\Temp\\jl_dn0CoZ\\subdir\\foo" size: 0 bytes device: 344850202 inode: 75265 mode: 0o100666 (-rw-rw-rw-) nlink: 1 uid: 0 gid: 0 rdev: 0 blksz: 4096 blocks: 0 mtime:  (just now) ctime:  (just now))
      From worker 11:	StatStruct("C:\\Users\\julia\\AppData\\Local\\Temp\\jl_dn0CoZ\\subdir\\foo" size: 0 bytes device: 344850202 inode: 75265 mode: 0o100444 (-r--r--r--) nlink: 1 uid: 0 gid: 0 rdev: 0 blksz: 4096 blocks: 0 mtime:  (just now) ctime:  (just now))
      From worker 11:	StatStruct("C:\\Users\\julia\\AppData\\Local\\Temp\\jl_dn0CoZ\\subdir\\foo" size: 0 bytes device: 344850202 inode: 75265 mode: 0o100666 (-rw-rw-rw-) nlink: 1 uid: 0 gid: 0 rdev: 0 blksz: 4096 blocks: 0 mtime:  (just now) ctime:  (just now))
      From worker 11:	StatStruct("C:\\Users\\julia\\AppData\\Local\\Temp\\jl_dn0CoZ\\subdir" size: 0 bytes device: 344850202 inode: 75248 mode: 0o040666 (drw-rw-rw-) nlink: 1 uid: 0 gid: 0 rdev: 0 blksz: 4096 blocks: 0 mtime:  (just now) ctime:  (just now))
      From worker 11:	StatStruct("C:\\Users\\julia\\AppData\\Local\\Temp\\jl_dn0CoZ\\subdir" size: 0 bytes device: 344850202 inode: 75248 mode: 0o040666 (drw-rw-rw-) nlink: 1 uid: 0 gid: 0 rdev: 0 blksz: 4096 blocks: 0 mtime:  (just now) ctime:  (just now))
      From worker 11:	StatStruct("C:\\Users\\julia\\AppData\\Local\\Temp\\jl_dn0CoZ\\subdir\\foo" size: 0 bytes device: 344850202 inode: 75265 mode: 0o100666 (-rw-rw-rw-) nlink: 1 uid: 0 gid: 0 rdev: 0 blksz: 4096 blocks: 0 mtime:  (just now) ctime:  (just now))
file                                            (11) |         failed at 2024-02-17T22:45:00.466
Test Failed at C:\buildkite-agent\builds\win2k22-amdci6-0\julialang\julia-master\julia-bca1214960\share\julia\test\file.jl:1682
  Expression: Sys.iswriteable(fpath)
Test Failed at C:\buildkite-agent\builds\win2k22-amdci6-0\julialang\julia-master\julia-bca1214960\share\julia\test\file.jl:1687
  Expression: Sys.iswriteable(fpath)
Test Failed at C:\buildkite-agent\builds\win2k22-amdci6-0\julialang\julia-master\julia-bca1214960\share\julia\test\file.jl:1696
  Expression: !(Sys.isreadable(fpath))
Test Failed at C:\buildkite-agent\builds\win2k22-amdci6-0\julialang\julia-master\julia-bca1214960\share\julia\test\file.jl:1697
  Expression: Sys.iswriteable(fpath)
Test Failed at C:\buildkite-agent\builds\win2k22-amdci6-0\julialang\julia-master\julia-bca1214960\share\julia\test\file.jl:1708
  Expression: Sys.iswriteable(fpath)
  • The access bit patterns are either 666 or 444, although we try to set it to 644, 755, 444, 244 during testing.
  • Despite always being 666 or 444, the isexecutable test all pass (why?)

I guess these problems are related to

So can Sys.iswriteable, Sys.isreadable even be made to work on windows?

@vtjnash
Copy link
Member

vtjnash commented Feb 19, 2024

You are welcome to pretty much just skip the broken tests on Windows and we can merge this. The permission bits are a bit different (and separate) from the ACLs, so it can be possible to see slightly behaviors, as well as the posix bits not mapping exactly to the Windows ACL, so some mappings may not be unique

@fatteneder
Copy link
Member Author

fatteneder commented Feb 19, 2024 •
edited
Loading

Ok. I went ahead and added another note to the doc strings.

@fatteneder fatteneder merged commit ea2b255 into JuliaLang:master Feb 20, 2024
7 checks passed
@fatteneder fatteneder deleted the fa/sys-read-write branch February 20, 2024 16:59
@fatteneder fatteneder mentioned this pull request Feb 25, 2024
Merged
@KristofferC KristofferC mentioned this pull request Feb 26, 2024
Merged
28 tasks
KristofferC pushed a commit that referenced this pull request Feb 26, 2024
@fatteneder @KristofferC
Add Sys.isreadable, Sys.iswriteable, update ispath (#53320)
945c93f 
As discussed here:
#53286 (comment)

Readds the methods that were removed in
#12819.

(cherry picked from commit ea2b255)
KristofferC added a commit that referenced this pull request Mar 1, 2024
@KristofferC
Backports release 1.11 (#53472)
2911937 
Backported PRs:
- [x] #53361 <!-- 🤖 [master] Bump the SparseArrays stdlib from c9f7293
to cb602d7 -->
- [x] #53300 <!-- allow external absint to hold custom data in
`codeinst.inferred` -->
- [x] #53342 <!-- Add `Base.wrap` to docs -->
- [x] #53372 <!-- Silence warnings in `test/file.jl` -->
- [x] #53357 <!-- 🤖 [master] Bump the Pkg stdlib from 6dd0e7c9e to
76070d295 -->
- [x] #53373 <!-- fix sysimage-native-code=no option with pkgimages -->
- [x] #53333 <!-- More consistent return value for annotations, take 2
-->
- [x] #53354 <!-- fix code coverage bug in tail position and `else` -->
- [x] #53407 <!-- fix sysimage-native-code=yes option -->
- [x] #53388 <!-- Fix documentation: thread pool of main thread -->
- [x] #53355 <!-- Fix synchronization issues on the GC scheduler. -->
- [x] #53429 <!-- Subtype: skip slow-path in `local_∀_∃_subtype` if
inputs contain no ∃ typevar. -->
- [x] #53437 <!-- Add debug variant of loader_trampolines.o -->
- [x] #53284 <!-- Add annotate! method for AnnotatedIOBuffer -->
- [x] #53466 <!-- [MozillaCACerts_jll] Update to v2023-12-12 -->
- [x] #53467 <!-- [LibGit2_jll] Update to v1.7.2 -->
- [x] #53326 <!-- RFC: when loading code for internal purposes, load
stdlib files directly, bypassing DEPOT_PATH, LOAD_PATH, and stale checks
-->
- [x] #53332
- [x] #53320 <!-- Add `Sys.isreadable, Sys.iswriteable`, update `ispath`
-->
- [x] #53476

Contains multiple commits, manual intervention needed:
- [ ] #53285 <!-- Add update mechanism for Terminfo, and common
user-alias data -->

Non-merged PRs with backport label:
- [ ] #53424 <!-- yet more atomics & cache-line fixes on work-stealing
queue -->
- [ ] #53408 <!-- task splitting: change additive accumulation to
multiplicative -->
- [ ] #53403 <!-- Move parallel precompilation to Base -->
- [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` -->
- [ ] #53391 <!-- Default to the medium code model in x86 linux -->
- [ ] #53125 <!-- coverage: count coverage where explicitly requested by
inference only -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Mar 1, 2024
tecosaur pushed a commit to tecosaur/julia that referenced this pull request Mar 4, 2024
@fatteneder @tecosaur
Add Sys.isreadable, Sys.iswriteable, update ispath (JuliaLang#53320)
499d13e 
As discussed here:
JuliaLang#53286 (comment)

Readds the methods that were removed in
JuliaLang#12819.
@JeffBezanson
Copy link
Member

  • These are currently separate functions, but should they be methods of Base.isreadable?
  • It seems like these should go in Filesystem (filesystem.jl) instead of sysinfo?

@JeffBezanson
Copy link
Member

It looks like they are there following isexecutable, but that function shouldn't be there either :) I think they should all move to Filesystem, with aliases for compatibility as needed.

@fatteneder fatteneder mentioned this pull request Mar 11, 2024
Merged
fatteneder added a commit that referenced this pull request Mar 13, 2024
@fatteneder
Move isexecutable, isreadable, iswritable to filesystem.jl (#53699)
8d31f33 
This PR migrates the methods `isexecutable, isreadable, iswritable` from
`Sys` to `Base`, but also generates an alias in `Sys` for backwards
compatibility.

Furthermore, `iswriteable` is renamed to `iswritable` in order to match
the already existing `Base.iswritable` method.

Suggested in
#53320 (comment).
KristofferC pushed a commit that referenced this pull request Mar 16, 2024
@fatteneder @KristofferC
Move isexecutable, isreadable, iswritable to filesystem.jl (#53699)
b9aaff5 
This PR migrates the methods `isexecutable, isreadable, iswritable` from
`Sys` to `Base`, but also generates an alias in `Sys` for backwards
compatibility.

Furthermore, `iswriteable` is renamed to `iswritable` in order to match
the already existing `Base.iswritable` method.

Suggested in
#53320 (comment).

(cherry picked from commit 8d31f33)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash approved these changes

Assignees
No one assigned
Labels
filesystem Underlying file system and functions that use it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fatteneder @vtjnash @JeffBezanson @KristofferC