dhall-lang / dhall-haskell

Maintainable configuration files
https://dhall-lang.org/
BSD 3-Clause "New" or "Revised" License
908 stars 211 forks source link

Support template-haskell 2.20.0 and unix-compat 0.7 #2496

Closed locallycompact closed 8 months ago

locallycompact commented 1 year ago

Caught by https://gitlab.horizon-haskell.net/package-sets/horizon-core/-/jobs/197815

Gabriella439 commented 1 year ago

Can you explain the unix-related change a bit more? The reason I ask is because I thought the unix package was unavailable on Windows (thus the dependence on unix-compat)

locallycompact commented 1 year ago

Oh, right. In that case this fix is probably wrong. I'm not sure why PosixCompat.User was removed in 0.7, then. The Changelog doesn't say.

locallycompact commented 1 year ago

Regarding https://github.com/haskell-pkg-janitors/unix-compat/issues/2#issuecomment-1487446182 . What is expected of these codepaths on Windows?

Gabriella439 commented 1 year ago

I'm going to check if the corresponding dhall subcommand is broken on windows; if so in the worst case scenario we can just remove support for that subcommand on Windows

Gabriella439 commented 1 year ago

Yeah, so it looks like some of the functionality that dhall to-directory-tree depended on from System.Posix.Compat was broken on Windows. In the common case users don't hit this code path (it's a rarely-used feature of this subcommand) which is probably why nobody noticed this problem just yet.

I think the best approach here is to disable that code path on Windows and I can put up the change for that.

Gabriella439 commented 1 year ago

Actually, it seems like there are some tests that depended on Unix permissions that were running successfully on Windows. That seems surprising because as far as I can tell those tests would have transitively run System.PosixCompat.User.get{User,Group}EntryFromName and failed:

https://github.com/dhall-lang/dhall-haskell/blob/7b40b1cebb1eb9d16358edf2cbfcafbd2c9efdf8/dhall/tests/Dhall/Test/DirectoryTree.hs#L32

cc: @mmhat: I wanted to know if you had any insight here.

mmhat commented 1 year ago

@Gabriella439 I'll have a look.

mmhat commented 1 year ago

Ok, the test didn't fail because we never tried to actually set the user or group in fixpointedUserGroup: This test just checks that we can parse the encoded directory tree and doesn't interpret it. That is due to the restrictions on the various platforms: On Windows, the implementation of unix-compat simply doesn't work while on Linux the restriction of chown(2) makes it difficult to test. From the man page:

The owner of a file may change the group of the file to any group of which that owner is a member.

That means, writing a proper test for that would mean that we make assumptions about the groups the user running the test is a member of... Unless I am missing something here.

Since metadata is broken on Windows I think the best is to disable it there and throw an error if the user attempts to set it explicitly: https://github.com/dhall-lang/dhall-haskell/pull/2507

Gabriella439 commented 1 year ago

@locallycompact: If you merge https://github.com/locallycompact/dhall-haskell/pull/1 into your branch then I think it will fix the build failure on Windows

locallycompact commented 1 year ago

Awesome thanks.

Incidentally though this th fix is wrong though because it's 2.11.0 that should be the future-proof bound. However, PvP isn't respected for unreleased commits on ghc master, 2.11.0 doesn't exist yet, so I'm rethinking how to approach future proofing.

I'll resubmit what can be merged.

ysangkok commented 1 year ago

@locallycompact Are you still working on this? Dhall was remove from Stackage because of this issue.

locallycompact commented 1 year ago

I will tidy this up, I'm a little indisposed atm but by all means feel free to do it before me.

locallycompact commented 8 months ago

Closed in favor of https://github.com/dhall-lang/dhall-haskell/pull/2541