-
Notifications
You must be signed in to change notification settings - Fork 368
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 installation order when processing .install files #4667
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good! See my comment on interrogations about special cases for #4494 though.
in | ||
let () = | ||
try if Unix.((lstat dst).st_kind <> S_REG) then | ||
remove_file dst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the right behaviour, but now I am wondering what happens if a package overwrites a directory installed by another package with a file 🤔
It shouldn't be worse than just overriding files, and since this DirTrack
is done outside of this, it should correctly detect the deleted files; but this has consequences with #4494, since that only handles lists of files expected to be installed: it should also track anything below in case it encounters a directory right ?
That seems pretty minor, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what happens at the higher level, but OpamSystem.install
will fail if the destination is a directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several kind of files are written in the install process:
- the
pkg.config
file that is package specific by name - the files in
.install
file that are written inprefix/pkg/xxx
- the misc files in
.install
that need confirmation from user to be installed, as they are absolute paths
Other install/copy are done via install
field commands not opam instructions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what happens at the higher level, but OpamSystem.install will fail if the destination is a directory?
indeed, it is the first check
let install ?(warning=default_install_warning) ?exec src dst =
if Sys.is_directory src then
internal_error "Cannot install %s: it is a directory." src;
if (try Sys.is_directory dst with Sys_error _ -> false) then
internal_error "Cannot install to %s: it is a directory." dst;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original PR in #4494 was significantly changed so none of this is my code (this also explains why i didn't see these issues sooner) and I can't really review it properly, but from afar it looks ok.
#4657 exposed no fewer than 3 bugs! 🐛
OpamSystem.install
doesn't the file permissions correctly if the file already exists (sinceopen_out_gen
only uses the mode if it actually creates the file). The first commit here fixes that and also the case of a symlink being overwritten which is handled byOpamSystem.copy_file
but wasn't being handled byOpamSystem.install
.opam-devel.install
installs a file calledopam
twice tolib/opam-devel
(fixed in Fix opam-devel.install #4664)NB
List.flatten
blows the stack w.r.t. the number of lists (which is fundamentally small) and the length of the longest sub-list - this would already blow the stack in an earlierList.map
call, so I convinced myself that a crazy opam .install file with gazillions of entries is already exploding.