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

System.Posix.Terminal.PosixString: Fix imports in HAVE_OPENPTY path #312

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Feb 1, 2024

System.OsPath.Data.ByteString.Short no longer exists as of filepath-1.5 and the os-string split.

@Bodigrim
Copy link
Contributor

Bodigrim commented Feb 1, 2024

@bgamari ci-wasm32-wasi used to work just a week ago (https://github.com/haskell/unix/actions/runs/7624299549/job/20769378162), but fails after your changes. Is it expected?

@bgamari
Copy link
Contributor Author

bgamari commented Feb 1, 2024

This is surprising. @TerrorJack, do you have any insight into what might be going on here?

import System.OsString.Internal.Types (PosixString(..))
#if MIN_VERSION_filepath(1,5,0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#if MIN_VERSION_filepath(1,5,0)
#ifdef MIN_VERSION_os_string

would be more robust, I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bgamari did you try this?

@hasufell
Copy link
Member

hasufell commented Feb 2, 2024

The wasm action downloads GHC from https://gitlab.haskell.org/ghc/ghc-wasm-meta/-/archive/master/ghc-wasm-meta-master.tar.gz , which I consider fragile. I'd much rather rely on GHCup here.

@TerrorJack
Copy link
Contributor

@bgamari This is sufficient to fix the wasm32 job:

diff --git a/cabal.project.wasm32-wasi b/cabal.project.wasm32-wasi
index 87a3e35..598e0f2 100644
--- a/cabal.project.wasm32-wasi
+++ b/cabal.project.wasm32-wasi
@@ -1,6 +1,7 @@
 packages: .
 
 package unix
+  flags: +os-string
   ghc-options: -Wno-unused-imports
 
 write-ghc-environment-files: always

I believe other platforms should also follow suit since +os-string is the config when using unix as a boot lib.

@hasufell
Copy link
Member

hasufell commented Feb 2, 2024

@TerrorJack if wasm doesn't compile with no os-string flag set, then that is a bug. This flag is automatic. There should be no need to toggle it.

If the cabal solver accepts -f-os-string, then the compilation must succeed.

@TerrorJack
Copy link
Contributor

@hasufell This then:

diff --git a/cabal.project.wasm32-wasi b/cabal.project.wasm32-wasi
index 87a3e35..f6e34e2 100644
--- a/cabal.project.wasm32-wasi
+++ b/cabal.project.wasm32-wasi
@@ -5,4 +5,4 @@ package unix
 
 write-ghc-environment-files: always
 
-allow-newer: all:base, all:filepath
+allow-newer: all:base

The wasm build failure in this PR is caused by cabal picking an invalid build plan with -f-os-string and filepath-1.5.2.0.

@hasufell
Copy link
Member

hasufell commented Feb 2, 2024

The wasm build failure in this PR is caused by cabal picking an invalid build plan with -f-os-string and filepath-1.5.2.0.

That doesn't explain it.

cabal build -w ghc-9.4.8 -f-os-string --constraint='filepath >= 1.5.2.0' causes a solver failure here (as it should):

Resolving dependencies...
Error: cabal: Could not resolve dependencies:
[__0] trying: unix-2.8.5.0 (user goal)
[__1] trying: unix:-os-string
[__2] next goal: filepath (dependency of unix +/-os-string)
[__2] rejecting: filepath-1.4.2.2/installed-1.4.2.2 (constraint from command
line flag requires >=1.5.2.0)
[__2] rejecting: filepath-1.5.2.0 (conflict: unix -os-string =>
filepath>=1.4.100.0 && <1.5.0.0)
[__2] rejecting: filepath-1.4.300.1, filepath-1.4.2.2, filepath-1.4.2.1,
filepath-1.4.2, filepath-1.4.1.2, filepath-1.4.1.1, filepath-1.4.1.0,
filepath-1.4.0.0, filepath-1.3.0.2, filepath-1.3.0.1, filepath-1.3.0.0,
filepath-1.2.0.1, filepath-1.2.0.0, filepath-1.1.0.4, filepath-1.1.0.3,
filepath-1.1.0.2, filepath-1.1.0.1, filepath-1.1.0.0, filepath-1.0,
filepath-1.5.0.0, filepath-1.4.200.1, filepath-1.4.200.0, filepath-1.4.100.4,
filepath-1.4.100.3, filepath-1.4.100.2, filepath-1.4.100.1, filepath-1.4.100.0
(constraint from command line flag requires >=1.5.2.0)
[__2] fail (backjumping, conflict set: filepath, unix, unix:os-string)
After searching the rest of the dependency tree exhaustively, these were the
goals I've had most trouble fulfilling: unix, filepath, unix:os-string

Edit: ah right, we have this:

    if flag(os-string)
      build-depends: filepath >= 1.5.0.0, os-string >= 2.0.0
    else
      build-depends: filepath >= 1.4.100.0 && < 1.5.0.0

And the allow-newer: filepath somehow makes it pick the -os-string path, but then ignore the upper bound. Ugh.

@bgamari
Copy link
Contributor Author

bgamari commented Feb 2, 2024

Hopefully the push above should fix this.

cabal.project.wasm32-wasi Outdated Show resolved Hide resolved
This appears to have broken with the `os-string` split of
`filepath-1.5`.
@bgamari
Copy link
Contributor Author

bgamari commented Feb 2, 2024

@hasufell do I need to worry about the FreeBSD failure here?

@Bodigrim
Copy link
Contributor

Bodigrim commented Feb 2, 2024

@bgamari I think FreeBSD just needs a version bump, similar to haskell/bytestring@6c6d81a

@hasufell
Copy link
Member

hasufell commented Feb 3, 2024

@bgamari I think FreeBSD just needs a version bump, similar to haskell/bytestring@6c6d81a

Probably not. I'm trying to get rid of cirrus CI. I already have two self hosted freebsd runners.

@bgamari
Copy link
Contributor Author

bgamari commented Feb 5, 2024

@hasufell , what needs to happen to merge this?

@hasufell
Copy link
Member

hasufell commented Feb 5, 2024

@bgamari are you asking for a release? I recently replied to an email about GHC release process and boot library bumps, but never got an answer.

@bgamari
Copy link
Contributor Author

bgamari commented Feb 5, 2024

No, I am merely asking for this to be merged so that I can bump the GHC submodule reference.

@hasufell
Copy link
Member

hasufell commented Feb 5, 2024

No, I am merely asking for this to be merged so that I can bump the GHC submodule reference.

You're aware we don't want GHC to ship non-released unix versions though?

@bgamari
Copy link
Contributor Author

bgamari commented Feb 5, 2024

I recently replied to an email about GHC release process and boot library bumps, but never got an answer.

Apologies, I had missed the questions in that email. I will reply on the thread.

You're aware we don't want GHC to ship non-released unix versions though?

Yes, I am aware of this. However, it is common practice for GHC upstream to point its submodules at unreleased submodule commits. These are bumped to released versions as maintainers are able to finalize their releases over the course of the GHC release process.

In my coordination email I suggested the timeframe on which we would like this to happen: have a release prior to alpha 2, slated for the second week of March 2024. Does this seem reasonable to you?

@hasufell hasufell merged commit 00bf6dd into haskell:master Feb 5, 2024
23 checks passed
@bgamari bgamari deleted the wip/filepath-1.5 branch February 5, 2024 14:37
@bgamari
Copy link
Contributor Author

bgamari commented Feb 5, 2024

Thanks, @hasufell !

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

Successfully merging this pull request may close these issues.

4 participants