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.

Sign up for GitHub

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

Jump to bottom

Add PosixFilePath and friends support (for AFPP) #202

Merged
merged 2 commits into from
Jul 17, 2022
Merged

Add PosixFilePath and friends support (for AFPP) #202

merged 2 commits into from
Jul 17, 2022

Conversation

hasufell
Copy link
Member

@hasufell hasufell commented Mar 22, 2022 •
edited
Loading

Depends on: haskell/filepath#103

For testing simple file operations, you can use https://github.com/hasufell/file-io (also please review the posix module)

@Bodigrim

@hasufell hasufell mentioned this pull request Mar 22, 2022
Merged
9 tasks
hasufell
hasufell commented Mar 22, 2022
View reviewed changes
cabal.project Outdated
source-repository-package
type: git
location: https://github.com/hasufell/filepath.git
tag: babf4068bd7e3c84b87033ed12542a34efa6a462
Copy link
Member Author

Choose a reason for hiding this comment

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

temporary

@hasufell hasufell marked this pull request as draft March 22, 2022 20:54
@hasufell hasufell marked this pull request as ready for review March 22, 2022 21:04
@hasufell hasufell force-pushed the AFPP branch 4 times, most recently from b7f1a64 to 2a96025 Compare March 24, 2022 19:44
@hasufell hasufell mentioned this pull request Apr 15, 2022
Open
@Bodigrim
Copy link
Contributor

@hasufell could you please rebase?

Bodigrim
Bodigrim reviewed Apr 17, 2022
View reviewed changes
unix.cabal Outdated Show resolved Hide resolved
@hasufell hasufell force-pushed the AFPP branch 2 times, most recently from c98b694 to 4d7bce9 Compare May 1, 2022 07:48
@hasufell
Copy link
Member Author

hasufell commented May 1, 2022

ping

@hasufell hasufell force-pushed the AFPP branch 3 times, most recently from 76b3442 to c0fd56c Compare May 1, 2022 17:18
@hasufell hasufell mentioned this pull request May 8, 2022
Merged
hs-viktor
hs-viktor reviewed May 30, 2022
View reviewed changes
Copy link
Contributor

@hs-viktor hs-viktor left a comment

Choose a reason for hiding this comment

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

Looks fine with minor nits. Please address those you don't object to, and rebase...

System/Posix/IO/PosixString.hsc Outdated Show resolved Hide resolved
System/Posix/PosixString.hs Show resolved Hide resolved
cabal.project Show resolved Hide resolved
@hasufell hasufell mentioned this pull request Jun 1, 2022
Closed
@Bodigrim
Copy link
Contributor

Bodigrim commented Jun 2, 2022

@hasufell could you please rebase?

@hasufell
Copy link
Member Author

hasufell commented Jun 2, 2022

@hasufell could you please rebase?

done

@vdukhovni
Copy link

Now that we have CI for "wasm", it is no longer in the way of routine work by wasm-agnostic maintainers. :-(
What's to be done about this? Can the wasm CI be enabled only on wasm branches? Or otherwise not get in the way?

@Bodigrim
Copy link
Contributor

Bodigrim commented Jun 2, 2022

I’m AFK, but either Wasm CI job should honor settings in cabal.project, or filepath released officially. @vdukhovni are you happy with this branch otherwise?

@hasufell hasufell force-pushed the AFPP branch 2 times, most recently from 567811d to c7c32da Compare June 2, 2022 20:58
@hs-viktor
Copy link
Contributor

The ARM CI isn't working yet, and I should look a bit more carefully at the new Posix-flavoured Terminal.hsc.

@hasufell hasufell mentioned this pull request Jul 10, 2022
Closed
5 tasks
@hasufell hasufell added this to the 2.8.0.0 milestone Jul 10, 2022
@Bodigrim
Copy link
Contributor

@TerrorJack ci-wasm32-wasi job does not seem to work any longer. Could you please take a look?

@TerrorJack
Copy link
Contributor

Sure I'll open a PR against master to fix the job.

@TerrorJack
Copy link
Contributor

@Bodigrim It should work now if you restart the job; I looked into it, and it seems to be a transient network failure; I added retry logic to the curl scripts.

@hasufell
Copy link
Member Author

Not due to a network failure: https://github.com/haskell/unix/runs/7280567595?check_suite_focus=true

I don't know what wasm32-wasi-cabal, but it clearly is not the same as cabal. Why do we have this even?

@TerrorJack
Copy link
Contributor

It's now a cabal bound issue, since ghc-wasm32-wasi uses a ghc-head build. I'm looking into relaxing bounds in the wasm32 cabal.project file atm.

@hasufell
Copy link
Member Author

We might have been hit by https://gitlab.haskell.org/ghc/ghc/-/issues/21738

@TerrorJack
Copy link
Contributor

@hasufell Thanks for the pointer! Given filepath is vendored in template-haskell in latest head which should resolve this issue, I'll proceed to update ghc-wasm32-wasi to latest head to unblock CI, but that'll take a couple more hours, are you fine with this timeframe?

@TerrorJack
Copy link
Contributor

To provide more context, the job is to added to test unix support for wasm32-wasi platform, upcoming in 9.6, with more details in https://gitlab.haskell.org/ghc/ghc/-/wikis/WebAssembly-backend

@hasufell
Copy link
Member Author

@hasufell Thanks for the pointer! Given filepath is vendored in template-haskell in latest head which should resolve this issue, I'll proceed to update ghc-wasm32-wasi to latest head to unblock CI, but that'll take a couple more hours, are you fine with this timeframe?

Yes

@TerrorJack
Copy link
Contributor

If you push the fix at https://gist.github.com/TerrorJack/6352f35dc53e7b1b1baef4391ff86c3d, the ci-wasm32-wasi job should have been fixed now.

@TerrorJack TerrorJack mentioned this pull request Jul 11, 2022
Merged
@hasufell hasufell force-pushed the AFPP branch 2 times, most recently from 8605076 to 2d86597 Compare July 12, 2022 17:29
hs-viktor
hs-viktor approved these changes Jul 16, 2022
View reviewed changes
Copy link
Contributor

@hs-viktor hs-viktor left a comment

Choose a reason for hiding this comment

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

LGTM. Modulo a pending rebase and ideally a successful CI run.

hasufell
hasufell commented Jul 16, 2022
View reviewed changes
System/Posix/IO/PosixString.hsc

import System.Posix.Types
import System.Posix.IO.Common
import System.Posix.IO.ByteString ( fdRead, fdWrite )
Copy link
Member Author

Choose a reason for hiding this comment

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

We import fdRead and fdWrite from ByteString now.

@Bodigrim
Copy link
Contributor

@hasufell there are several redundant imports. GHC build will choke on warnings.

@hasufell hasufell force-pushed the AFPP branch 2 times, most recently from be2f725 to 7f32a74 Compare July 16, 2022 23:14
Bodigrim
Bodigrim approved these changes Jul 17, 2022
View reviewed changes
Copy link
Contributor

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

Thanks, great!

@Bodigrim Bodigrim merged commit dd1f0aa into haskell:master Jul 17, 2022
@hasufell hasufell mentioned this pull request Jul 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hs-viktor hs-viktor hs-viktor approved these changes

@Bodigrim Bodigrim Bodigrim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.8.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@hasufell @Bodigrim @vdukhovni @hs-viktor @TerrorJack