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.

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

Fix Permission Denied on Win32 install operation #4827

Merged
merged 2 commits into from
Apr 4, 2022

Conversation

jonahbeckford
Copy link
Contributor

@jonahbeckford jonahbeckford commented Sep 5, 2021

Problem

The Windows preconditions are:

  • an Opam package already exists in the Opam switch
  • one of its files has read-only permissions, and
  • that file is individually "install"-ed by Opam

Then when the package is recompiled it will fail with Permission denied.

For example:

02:23.245  FILE(.install)         Wrote Z:\source\diskuv-ocaml-starter\build\dev\Debug\_opam\.opam-switch\install\dune.install in 0.000s
02:23.245  TRACK                  before install: 34 elements scanned in 0.000s
02:23.245  ACTION                 Installing dune.2.9.0.

02:23.245  SYSTEM                 install Z:\source\diskuv-ocaml-starter\build\dev\Debug\_opam\.opam-switch\build\dune.2.9.0\_build\install\default\bin\dune.exe -> Z:\source\diskuv-ocaml-starter\build\dev\Debug\_opam\bin\dune.exe (755)
02:23.245  PARALLEL               Exception while computing job 318106069: ✶ dune.2.9.0

#=== ERROR while installing dune.2.9.0 ========================================#
Sys_error("Z:\\source\\diskuv-ocaml-starter\\build\\dev\\Debug\\_opam\\bin\\dune.exe: Permission denied")
Backtrace:
  Raised at OpamParallel.Make.aux_map.loop.fail in file "src/core/opamParallel.ml", line 225, characters 8-76
  Called from OpamParallel.Make.aux_map.loop in file "src/core/opamParallel.ml", line 255, characters 48-56
  Called from OpamParallel.Make.aux_map in file "src/core/opamParallel.ml", line 293, characters 12-44
  Called from OpamParallel.Make.map in file "src/core/opamParallel.ml", line 301, characters 15-57
  Called from OpamSolution.parallel_apply in file "src/client/opamSolution.ml", line 664, characters 8-209

The file was initially installed read-only.

Specifically, from the Cygwin and Windows' perspectives the file and its ACL looks like:

$ ls -l $(cygpath 'Z:\source\diskuv-ocaml-starter\build\dev\Debug\_opam\bin\dune.exe')
-r-x------+ 1 beckf beckf 6927360 Sep  4 18:00 /cygdrive/z/source/diskuv-ocaml-starter/build/dev/Debug/_opam/bin/dune.exe

$ getfacl -e $(cygpath 'Z:\source\diskuv-ocaml-starter\build\dev\Debug\_opam\bin\dune.exe')
# file: /cygdrive/z/source/diskuv-ocaml-starter/build/dev/Debug/_opam/bin/dune.exe
# owner: beckf
# group: beckf
user::rwx
group::---              #effective:---
group:SYSTEM:rwx        #effective:---
group:Administrators:rwx        #effective:---
mask::---
other::---

and from the MSYS2 perspective the file looks like:

$ ls -l $(cygpath 'Z:\source\diskuv-ocaml-starter\build\dev\Debug\_opam\bin\dune.exe')
-r-xr-xr-x 1 beckf beckf 6927360 Sep  4 18:00 /z/source/diskuv-ocaml-starter/build/dev/Debug/_opam/bin/dune.exe

Solution

Do a chmod 640 and delete the file before trying to do a truncate+write into it.

@jonahbeckford jonahbeckford marked this pull request as ready for review September 5, 2021 01:39
@rjbou rjbou requested a review from dra27 November 9, 2021 10:06
@dra27 dra27 force-pushed the feature-win32-permission-denied branch 2 times, most recently from 330cc85 to ce9b5b1 Compare March 23, 2022 16:27
@dra27
Copy link
Member

dra27 commented Mar 23, 2022

Apologies for the delay looking at this! GitHub still doesn't let one do code suggestions anywhere in the file, so I've pushed a commit with my suggestions, but that commit may still be removed. My comments:

  • Unix.chmod is an emulation on Windows - it only controls the readonly bit, so I'd suggest 0o666 as that more accurately reflects what will happen (Dune does the same, although that might be down to shared authorship!)
  • In setup_copy, we should preserve the cp-like semantics, which are inode preserving (i.e. it truncates if possible). To this end, instead of always deleting the target on Windows, I instead moved the chmod change around the opening of the file. open_out_gen doesn't give very rich information, so instead of open_out_gen followed later by Unix.descr_of_out_channel, I switched to Unix.openfile and use Unix.out_channel_of_descr. This is safe for Windows as binary mode is wanted and no attempt will be made to change that.
  • The remove_file function could do with the same logic, so I added it there too (propagates to copy_file as well)

@dra27 dra27 added this to the 2.2.0~alpha milestone Mar 23, 2022
@dra27 dra27 added this to PR in progress in Opam 2.2.0 via automation Mar 23, 2022
@rjbou rjbou moved this from PR in progress to PR to review in Opam 2.2.0 Mar 23, 2022
@rjbou rjbou force-pushed the feature-win32-permission-denied branch from ce9b5b1 to 32226ed Compare March 24, 2022 11:48
@rjbou rjbou moved this from PR to review to PR finalised (merge with CI) in Opam 2.2.0 Mar 24, 2022
@rjbou rjbou force-pushed the feature-win32-permission-denied branch from 32226ed to bb8833c Compare April 1, 2022 09:02
@rjbou rjbou merged commit 47847d9 into ocaml:master Apr 4, 2022
Opam 2.2.0 automation moved this from PR finalised (merge with CI) to Done Apr 4, 2022
@rjbou
Copy link
Collaborator

rjbou commented Apr 4, 2022

Thanks!

@jonahbeckford jonahbeckford deleted the feature-win32-permission-denied branch May 3, 2022 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Opam 2.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants