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

Mark old dune releases as not compatible with windows #25484

Merged
merged 2 commits into from Mar 19, 2024

Conversation

raphael-proust
Copy link
Collaborator

fixes #25483

@mseri
Copy link
Member

mseri commented Mar 13, 2024

I agree with @emillon
As a compromise I think we can wait a few days and then merge this

@emillon
Copy link
Contributor

emillon commented Mar 14, 2024

Yes. Here is my original comment:

I don't understand the reasoning. This is something important to fix, but this is not a regression or anything. This is something that has never worked. The old versions of dune are working well for a bunch of windows users. Why force them to upgrade? The solver will pick the new version anyway.

In addition, dune started vendoring spawn in 3.0.0, so the older versions might not be affected.

@dra27
Copy link
Member

dra27 commented Mar 14, 2024

In #25474 (comment):

Well there was never any Windows users using opam-repository as opam-repository is not currently available on Windows. People currently use forks that are not currently updated.

There are users at the moment who have been advised (for some time) to use ocaml/opam-repository with the sunset branch of ocaml-opam/opam-repository-mingw. So there are users at the moment who have been installing versions of dune after 3.5.0 (the last version in opam-repository-mingw) who may suddenly get CI failures.

I think there's an easy compromise, though - instead of os != "win32", if it's os != "win32" | os-distribution = "cygwinports" then the packages remain installable when using OCaml for Windows. I agree it is a good idea not to have (new) users of opam 2.2.0 hitting this.

I also agree with @emillon that this affects Dune 3.0.0+ and 1.x and 2.x should therefore be left alone.

@raphael-proust
Copy link
Collaborator Author

I've pushed a commit to undo the changes on 1.* and 2.* versions.

@dra27 could you edit the wikipage https://github.com/ocaml/opam-repository/wiki/Depexts-os-distribution---os-family-values adding a table at the end for the different values of os and os-distribution that can appear on windows? Alternatively, if you just reply here with a list of the values and a brief explanation I can make the table.

@dra27
Copy link
Member

dra27 commented Mar 15, 2024

I can do, though do we want to expand the entire table to include the os value as well? The Cygwin part becomes very confusion here, as it's both an OS in its own right but also an os-distribution for native Windows.

  • os-family is windows for both native Windows and Cygwin (i.e. Sys.win32 || Sys.cygwinos-family = windows)
  • os is win32 for a native Windows build of opam (which includes OCaml for Windows).

For os-distribution, we then have:

Distribution os os-distribution os-family Notes
Native1 Windows win32 cygwin windows Depexts + Unix tools provided by Cygwin
Native Windows win32 msys22 windows Depexts + Unix tools provided by MSYS2
Native Windows win32 cygwinports windows Legacy "OCaml for Windows" (opam 2.0.10 only)
Cygwin cygwin cygwin windows Cygwin's own opam distribution

Footnotes

  1. "Native" here refers to programs able to run outside of Cygwin/MSYS2, principally from the Command Prompt or Windows PowerShell.

  2. At the moment, this is "tier 2" from opam's perspective - Cygwin is the primary supported mechanism for 2.2.0, but we're paying a lot of attention to MSYS2 partly because that's what Diskuv uses now and also because we expect we'll likely switch to recommending it across the board in future.

@benmandrew
Copy link
Contributor

I don't know if it's useful for the PR but the CI should finish this sometime Friday morning (CET). It generated an extraordinary number of jobs!

@mseri
Copy link
Member

mseri commented Mar 19, 2024

Thanks. I think we can release resources and merge, now one week has passed

@mseri mseri merged commit d34d070 into ocaml:master Mar 19, 2024
1 of 2 checks passed
@MSoegtropIMC
Copy link
Contributor

@mseri @raphael-proust : this PR breaks Coq Platform CI:

https://github.com/coq/platform/actions/runs/8352401423/job/22862430293#step:5:1683

There are a few 1000 users which have successfully used

dune.3.7.0
dune-configurator.3.7.0

on Windows (cygwin based MinGW cross build) and it is the pinned version of dune in Coq Platform 8.17~2023.08 - which for sure makes good use of dune, so that this is a tough test.

Please undo this change at least for version 3.7.0 asap.

MSoegtropIMC added a commit to MSoegtropIMC/platform that referenced this pull request Mar 20, 2024
mseri added a commit to mseri/opam-repository that referenced this pull request Mar 20, 2024
This is currently breaking windows installations of coq,
see ocaml#25484

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
@dra27
Copy link
Member

dra27 commented Mar 20, 2024

I think there's an easy compromise, though - instead of os != "win32", if it's os != "win32" | os-distribution = "cygwinports" then the packages remain installable when using OCaml for Windows. I agree it is a good idea not to have (new) users of opam 2.2.0 hitting this.

The Coq platform uses OCaml for Windows, and this suggestion would have completely avoided this breakage.

@mseri
Copy link
Member

mseri commented Mar 20, 2024

That was my oversight, I thought the PR was modified also to include that (but in any case I had no clue if this would have made a difference for coq or not).

@dra27 I can make the change still for the affected versions, the merge was very recent so it could prevent further breakages. What do you think? The change is ready here: https://github.com/mseri/opam-repository/pull/new/relax-dune

@mseri
Copy link
Member

mseri commented Mar 28, 2024

@jonahbeckford is this creating issues with DKML (which uses dune 3.12.1)?
Should I relax the requirements on that dune version, with something like #25529 or you can use the new fixed 3.14.2 version in dkml?

@jonahbeckford
Copy link
Contributor

is this creating issues with DKML (which uses dune 3.12.1)?

Yes. DkML started using unpatched Dune at 3.8.3. Changes to dune in opam-repository can now break DkML users. And the availability conditions os != "win32" | os-distribution = "cygwinports" excludes DkML.

with something like #25529 or you can use the new fixed 3.14.2 version in dkml?

I cannot force people to upgrade to 3.14.2.

From the user perspective: It is a terrible UX and I have no way of contacting the users to notify them.

From the resource perspective: cutting a new version of DkML is very expensive for me ... it takes a week of my time and at least a month of soak testing to ensure the package + versions work well together for the users. That expense will continue until the day that opam CI gates each new package version against MSVC, just like today if an opam package version fails on Linux opam CI that package version is not permitted in the opam repository.

From a technical perspective: The "most important" packages (~100) are pinned in DkML to minimize my support burden, to mitigate the ecosystem wide lack of testing for native Windows, and because I'm opinionated that everything should be versioned by default. This thread is close to convincing me that I should have been versioning the opam repository as well.

So ... can we please roll back soon?

Emergency mitigation: Although I am having serious difficulty figuring out what happened and why ... and I'm concerned by that Discuss #windows-support thread ... if a rollback can't occur on the opam repository than please someone do the equivalent of a git revert PR in https://github.com/diskuv/diskuv-opam-repository/tree/main/packages. After that I can retag it for DkML 2.10. Every DkML switch has diskuv-opam-repository at a higher rank so this would be invisible for users.

mseri added a commit to mseri/diskuv-opam-repository that referenced this pull request Mar 28, 2024
mseri added a commit to mseri/diskuv-opam-repository that referenced this pull request Mar 28, 2024
@mseri
Copy link
Member

mseri commented Mar 28, 2024

@jonahbeckford I have sent a PR on the fly, while we rediscuss this issue

@dra27
Copy link
Member

dra27 commented Mar 29, 2024

@jonahbeckford - which bit of the #windows-support thread on Discord are you referring to? (sorry, I don't use Discord very frequently, and I'm struggling to find the context).

On the CI front, as I noted in #25536 (comment), I think the lack of CI here (ocaml/opam-repository) is being over-blown for this issue. For good or ill, these packages were intentionally disabled in this PR - CI would simply have told us that they'd been disabled. The error has been one of human logic.

Having already pointed out one error in the logic behind "Well there was never any Windows users using opam-repository as opam-repository is not currently available on Windows.", we now have two. Given that Dune 3.x survived for quite some time before we discovered a user with a non-ASCII character in their user ID, rather than repeatedly patching these packages, I'm rather on the side of completely reverting this. If we get continuing reports of problems, we could re-apply the post-messages filter in #25529 to 3.0.x-3.14.0 incl., but I think it's quite clear that trying to patch these older packages has caused more problems than it's solved.

@emillon
Copy link
Contributor

emillon commented Mar 29, 2024

Given that Dune 3.x survived for quite some time before we discovered a user with a non-ASCII character in their user ID, rather than repeatedly patching these packages, I'm rather on the side of completely reverting this.

Agree. This change breaks dune for users that were unaffected, and affected users would install the latest version by default anyway.

@mseri
Copy link
Member

mseri commented Mar 29, 2024

I am not on my laptop until monday, @raphael-proust could you revert this PR and instead add just a message on those dune versions like

post-messages: [
  "Please note that this version of dune does not support unicode characters on Windows and may behave unexpectedly, consider updating to version 3.14.2 or greater" {os = "win32" & os-distribution="cygwinports"}
]

@mseri
Copy link
Member

mseri commented Mar 29, 2024

Then we can merge the revert as soon as the linter is green

@emillon
Copy link
Contributor

emillon commented Mar 29, 2024

Sorry to nitpick, but I don't think that "does not support unicode characters on Windows" is an accurate issue of the problem. It crashes when the environment contains something other than ASCII. You could point at ocaml/dune#10180, but really I find post-messages are pretty noisy. We are not listing all the open issues for a given dune release and I don't think that this one is special enough to be listed on every install.

@mseri
Copy link
Member

mseri commented Mar 29, 2024

I am fine with rewording it, but it is listed only on windows and only on the possibly affected installations. At least we warn the users, I think it is better than pretending it is fine and already informs them that weird crashes may be solved just by upgrading

@emillon
Copy link
Contributor

emillon commented Mar 29, 2024

I'll let you decide what's best in the context of opam-repository, but to be fair, wouldn't "did you try to upgrade the package" be the first thing to recommend in any support discussion?

@jonahbeckford
Copy link
Contributor

@jonahbeckford I have sent a PR on the fly, while we rediscuss this issue

Thanks for looking into that. Took too long for me to get to it. For now I'm not going to retag because my muscle memory is telling me that opam doesn't always see a change when a tag's commit changes. I could be making the problem worse by retagging.

The pinning of the opam repository that I put on https://discuss.ocaml.org/t/stuck-installing-dune-on-windows/14395/3?u=jbeckford is safer for now.


@jonahbeckford - which bit of the #windows-support thread on Discord are you referring to? (sorry, I don't use Discord very frequently, and I'm struggling to find the context).

On the CI front, as I noted in #25536 (comment), I think the lack of CI here (ocaml/opam-repository) is being over-blown for this issue. For good or ill, these packages were intentionally disabled in this PR - CI would simply have told us that they'd been disabled. The error has been one of human logic.

I couldn't follow the context on Discord very well either so let's skip that.

I wasn't aware of the CI issue you mentioned. Stepping back, the human curated opam repository and the resulting quality is amazing IMHO. But I know I'm not prescient, and I would need something to visually break if I was managing thousands of packages and the equivalent of rm -rf /windows is accidentally slipped in. My knee-jerk reaction is to say "CI", but I don't care about the particular mechanism as long as there is one.

samoht added a commit to samoht/opam-repository that referenced this pull request Apr 1, 2024
@samoht samoht mentioned this pull request Apr 1, 2024
@samoht
Copy link
Member

samoht commented Apr 1, 2024

I've reverted these changes here: #25609

The goal is to give us more time to find the appropriate fix and unbreak downstream Windows users.

samoht added a commit that referenced this pull request Apr 1, 2024
@mseri
Copy link
Member

mseri commented Apr 1, 2024

Thanks a lot @samoht!

@MSoegtropIMC
Copy link
Contributor

Comming back on this a bit late (was on vacation): the proposed fix using os != "win32" | os-distribution = "cygwinports" does not work for Coq Platform. Coq Platform uses Cygwin to build things, but this is a MinGW cross build and OCaml and dune are actually MinGW, so os-distribution = "cygwinports" does not apply to dune.

This is a bit or a weird configuration, but it is what evolved over the years and this is what works. In the end we definitely need a MinGW dune, because we create a cygwin free Windows installer and this needs a MinGW dune. One could discuss if we should have an intermediate cygwin MinGW cross dune, but last time I tried to build OCaml for cygwin, the 64 bit version appeared to be broken - and 32 bit cygwin does hardly exist any more.

@dra27
Copy link
Member

dra27 commented Apr 5, 2024

@MSoegtropIMC - os-distribution refers to opam, which I believe you're using from "OCaml from Windows". I just kicked off a build of Coq Platform "from sources" and opam var os-distribution is giving me cygwinports?

@MSoegtropIMC
Copy link
Contributor

True. But fact is that when I completely remove the restriction from the dune 3.7.0 package (as it was before this PR) Coq Platform 8.17 CI works - with the currently published opam package for dune.3.7.0 it doesn't. I will do a local build and see.

It might be some strange coincidence that something else broke around the time these changes have been done, but it is not that likely because the newer Coq Platform builds (which use newer dune) work fine. As I said, I will test it locally.

@MSoegtropIMC
Copy link
Contributor

To clarify: the Coq Platform 8.17 build does not work any more since I removed my local patch for dune 3.7.0 which restored the version prior to this PR. It did run fine before this PR and during the few days I used a local patched opam file for dune 3.7.0.

@samoht
Copy link
Member

samoht commented Apr 5, 2024

@MSoegtropIMC is the Coq platform still broken? I thought I completely reverted this PR with #25609 , but maybe I forgot something (if yes, it was my mistake -- as there were multiple changes happening after that PR had been merged, I had to revert it manually)

@mseri
Copy link
Member

mseri commented Apr 6, 2024

I had also double checked the PR as well as Jonah, and all the packages seemed addressed. Looking at it, 3.7.0 was properly reverted to no bound: https://github.com/ocaml/opam-repository/pull/25609/files#diff-ef6a2ce22378da0735d76b3235cafc19525f58c48976804639d5679e884cc584

@MSoegtropIMC
Copy link
Contributor

Sorry, there was some overlap - Coq Platform Windows CI failed until including April 1st with

unmet availability conditions: os != "win32" | os-distribution != "cygwinports"

and started to work again an April 2nd - so it did already work on April 5th when I reported it. I was picking up analysis on a local build I did a few days before ...

So all is well now. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make old dune versions unavailable on win32
8 participants