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

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

Closed
wants to merge 2 commits into from

Conversation

locallycompact
Copy link
Contributor

@locallycompact locallycompact changed the title Support template-haskell 2.20.0 Support template-haskell 2.20.0 and unix-compat 0.7 Mar 28, 2023
@Gabriella439
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Regarding haskell-pkg-janitors/unix-compat#2 (comment) . What is expected of these codepaths on Windows?

@Gabriella439
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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:

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

@mmhat
Copy link
Collaborator

mmhat commented Apr 12, 2023

@Gabriella439 I'll have a look.

@mmhat
Copy link
Collaborator

mmhat commented Apr 13, 2023

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: #2507

@Gabriella439
Copy link
Collaborator

@locallycompact: If you merge locallycompact#1 into your branch then I think it will fix the build failure on Windows

@locallycompact
Copy link
Contributor Author

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
Copy link

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

@locallycompact
Copy link
Contributor Author

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

@locallycompact
Copy link
Contributor Author

Closed in favor of #2541

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.

None yet

4 participants