-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Unison 2.53.8. #28851
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
base: master
Are you sure you want to change the base?
Unison 2.53.8. #28851
Conversation
|
There is something very weird happening with |
|
@jmid what is the correct syntaxt to be uninstallable on mingw? |
|
I had something similar happen: #28478 (comment) 🤷 My current understanding is that the Rather than
where we utilize the bottom table from here: https://github.com/ocaml/opam-repository/wiki/Depexts-os-distribution---os-family-values |
|
What's the values of the variables for the msvc port? |
dfb5611 to
7165ff4
Compare
|
The following did the trick: Not particularly elegant. |
|
Looks perfectly good to me :) |
The opam CI linter is failing with I don't know how serious this is though. Is it "just" a bad-practice warning or does it mean that it is somehow broken to do so? 🤔 |
|
I don’t know, @rjbou or @kit-ty-kate what do you think? |
mmh, how did you test this? Package variables aren't supposed to be evaluated in the I've tried the following for good measure, just in case there is something wrong with the code: but i'm unable to install it even if |
packages/unison/unison.2.53.8/opam
Outdated
| available: (os != "win32" | host-system-msvc:installed) | ||
| depends: [ | ||
| "ocaml" {>= "4.08"} | ||
| ] | ||
| depopts: [ | ||
| "lablgtk3" {>= "3.1.0"} | ||
| "ocamlfind" | ||
| "host-system-msvc" |
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.
Something like that would work instead, for example:
| available: (os != "win32" | host-system-msvc:installed) | |
| depends: [ | |
| "ocaml" {>= "4.08"} | |
| ] | |
| depopts: [ | |
| "lablgtk3" {>= "3.1.0"} | |
| "ocamlfind" | |
| "host-system-msvc" | |
| depends: [ | |
| "ocaml" {>= "4.08"} | |
| "host-system-msvc" {os = "win32"} | |
| ] | |
| depopts: [ | |
| "lablgtk3" {>= "3.1.0"} | |
| "ocamlfind" |
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 does not work. If I specify this, then opam simply uninstalls host-system-msys and installs host-system-msvc (at least on CI)...
7165ff4 to
591613f
Compare
|
Thanks. I did that change, let's see how it goes. |
|
Cc @tleedjarv : again, I don't have a Windows machine at hand, and it looks like Unison refuses to build on the MinGW port. Since the build script explicitly mentions the MSVC port, I restricted Windows support to MSVC. Please tel me if this is ok with you. |
591613f to
2548201
Compare
|
Alright, I tried to disable |
packages/unison/unison.2.53.8/opam
Outdated
| ] | ||
| conflicts: [ | ||
| "lablgtk3" {os = "win32"} | ||
| } |
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.
| } | |
| ] |
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.
To address the parse error:
Warning: Could not read file D:\opamroot\repo\default\packages\unison\unison.2.53.8\opam. skipping:
At D:\opamroot\repo\default\packages\unison\unison.2.53.8\opam:29:0-29:1::
Parse error
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.
Thanks. Where do you get these lints?
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.
(Somehow the "opam-ci" link disappeard for some time...)
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.
Thanks. Where do you get these lints?
Click the Windows CI / build ... link
https://github.com/ocaml/opam-repository/actions/runs/19137212003/job/54692236076?pr=28851
- then unfold the
Install packagesstep - it is then written at the top:
<><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><>
[default] synchronised from file://D:/a/opam-repository/opam-repository
Warning: Could not read file D:\opamroot\repo\default\packages\unison\unison.2.53.8\opam. skipping:
At D:\opamroot\repo\default\packages\unison\unison.2.53.8\opam:29:0-29:1::
Parse error
Now run 'opam upgrade' to apply any package updates.
- Testing unison-gui.2.53.8
unison-gui.2.53.8 is not installable. Skip.
- Testing unison.2.53.8
unison.2.53.8 is not installable. Skip.
You can further unfold the CI log for each individual package.
That's where I could see unison.2.53.8 building and unison-gui.2.53.8 failing.
This kind of error could also have been caught by running opam lint path/to/opam/file locally first (not pointing any fingers here - there's a reason I started sometimes remembering to do so... 😅 )
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.
(Somehow the "opam-ci" link disappeard for some time...)
Yes, I've noticed too (both yesterday and today). I've notified @mtelvers
2548201 to
359bde1
Compare
|
I'm not sure I understand the intent of limiting this to only MSVC on Windows. |
Well, apparently not. |
359bde1 to
1f124b1
Compare
Perhaps I am misunderstanding something. On commit 591613f I see both Windows workflows building Looking at the package I know from a self-PR that there doesn't seem to be much preventing it from installing on an existing setup with That being said, it still appears as if |
|
I posted a bug report on Unison for the build failure on MinGW: bcpierce00/unison#1164. Let's see what happens. It may be the case that I have a quick answer... |
|
Appart from that, CI is failing under FreeBSD. The issue is caused by a fialure to build Cairo (the FreeBSD package, it seems). I am even less a FreeBSD user than a Windows user, so I don't want to debug this. Should I leave the FreeBSD issue as is, or should I disable the GUI on FreeBSD? It seems to be more a FreeBSD issue than an Opam/Unison issue. |
|
It is fine to leave FreeBSD as is. These days it is failing more often than succeeding, so I believe it is a CI issue. |
|
Is it possible to get the log files of the opam build from the CI? I can get |
TUI/CLI (so |
Interesting. Did this change recently? I remember from packaging the previous versions that this was not possible easily. Also, does this work with nmake? And what about the installed files? Does
We can have a debate about what "incompatible" means in this context, but the end result is that Now, well, if @dra27 or the maintainers of the repo tell me to do otherwise, I will comply, of course. |
Should have been since 2.53.4. I'm not sure why it hasn't been brought up earlier.
It should, yes. If it doesn't then it's a bug.
Installs only the built files (otherwise it's a bug). There is currently no intersection between these targets. Shared between those are optional targets of fsmonitor and manpage (built with target |
228eb2e to
8f66b26
Compare
|
I also make sure that the native MacOS GUI is built on that system, but this fails on CI because xcode is not installed. I guess this is not an issue for merging? |
4f83f4b to
b482874
Compare
|
No need to build About the native macOS GUI. I don't know which GUI is more used or preferred by Mac users, but it is relevant to know that the native GUI is currently more or less unmaintained and at some point may deteriorate quickly with little notice. |
b482874 to
721845d
Compare
|
Fair enough. I fixed both the macos and the nmake issues. |
721845d to
9d89aa3
Compare
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.
The present version LGTM 👍
Thanks for the extra effort to get things running nicely on Windows too!
Would you consider adding x-maintenance-intent entries?
https://github.com/ocaml/opam-repository/blob/master/governance/policies/archiving.md
9d89aa3 to
e42dce3
Compare
|
So now MSYS2 CI fails because of lablgtk... |
|
That's all right imo. If and when lablgtk or other dependencies are fixed, we will see if this works or not. But I'd give it the benefit of doubt: we don't normally block a merge on an independent dependency that fails to install in the CI |
|
Thanks a lot for all the effort. Github does not allow me to merge until @dra27 explicitly approves the PR though |
dra27
left a comment
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 pre-stamping an approval to lift my "changes" block - just one last little change. With #28918, this should finally pass the MSYS2 CI!
e42dce3 to
b8c8db0
Compare
|
Great! Many thanks, @dra27, to take care of this, and being (positively) so picky about details. This is thanks to that that we get high-quality support of OCaml for Windows. I really appreciate that, and I think Unison developpers (@tleedjarv) apreciate that too! |
No description provided.