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

directory-1.3.8.0 violates the Haskell PVP #147

Closed
andreasabel opened this issue Nov 19, 2022 · 12 comments
Closed

directory-1.3.8.0 violates the Haskell PVP #147

andreasabel opened this issue Nov 19, 2022 · 12 comments
Labels
type: b-discussion This is a discussion with unclear objectives.

Comments

@andreasabel
Copy link
Member

From the directory-1.3.8.0 CHANGELOG:

Modules in directory are no longer considered Safe because the System.OsPath dependency is no longer Safe.

This means that all packages that use directory < 1.4 and import safe System.Directory are broken.

In the wild, this looks like this:

Building library for MissingH-1.5.0.1..
...
src/Network/Email/Sendmail.hs:35:1: error:
    System.Directory: Can't be safely imported!
    The module itself isn't safe.
   |
35 | import safe System.Directory
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^...
...
Error: cabal: Failed to build MissingH-1.5.0.1

Standard action path:

  • deprecate directory-1.3.8.0 on hackage
  • revise it to become unbuildable
  • release as directory-1.4

ATTN: @Rufflewind

@Rufflewind
Copy link
Member

@bgamari @hasufell System.OsPath is not Safe and so directory transitively also became non-Safe. @andreasabel considers this a PVP violation, which leaves us with two options:

  1. Restore the Safe status in System.OsPath, send out a minor release of filepath, and then mark filepath-1.4.100.0 as deprecated; or

  2. Retain the non-Safe status quo, send out a major release of directory-1.4.0.0, and then mark directory-1.3.8.0 as deprecated.

Any thoughts on which approach to take?

@hasufell
Copy link
Member

hasufell commented Nov 20, 2022

I feel the specification is not very clear on that matter

https://pvp.haskell.org/

@Bodigrim

I personally think that "compiles without code change" is not really what non-breaking or "other" changes from PVP spec demand.

Then again, if you consider "Safe" as part of the API, then you could argue different. The spec only implicitly defines "API".

@andreasabel
Copy link
Member Author

I agree that the PVP does not mention anything on safe vs. non-safe. While nothing violates the letter of the PVP, certainly going from safe to unsafe violates the spirit of the PVP.
The spirit (and dominant practice) is that it is sufficient to set a major version upper bound for your dependencies (with some exceptions, e.g. when you define orphan instances). So, if you make changes to your package that can break downstream packages, you need to bump the major version.

@hasufell
Copy link
Member

This would mean filepath itself also needs a major version bump. This is what I initially planned to do, but GHC team said that will create more work for them.

@bgamari @mpickering

@Bodigrim
Copy link
Contributor

There is a never ending trail of destruction caused by Safe Haskell. Friends do not let friends use {-# Safe #-}. I’m very serious, please just don’t.

@andreasabel
Copy link
Member Author

andreasabel commented Nov 20, 2022

Friends do not let friends use {-# Safe #-}.

This message should go to the haskell-language-server folks, summoning @jneira.
In my case (MissingH) the safe imports were added (Feb 2022) by the HLS tactic that creates explicit import lists (a terribly useful tool otherwise, maybe this is a flaw).

The GHC crowd seems rather undecided what to do with SafeHaskell, e.g. see this unresolved issue from 2014: https://gitlab.haskell.org/ghc/ghc/-/issues/8745

Reported upstream to PVP:

@Bodigrim
Copy link
Contributor

Bodigrim commented Nov 20, 2022

I suggest both directory and MissingH to remove Safe pragmas. This is a misfeature which makes everything too fragile.

Bumping major versions of filepath and directory is an extremely costly change for the ecosystem. According to the letter of PVP they are not in breach, and my opinion is that it's Safe Haskell as a feature who is at fault with PVP here. There are only handful users of Safe Haskell, and it seems less costly to revise them by adding directory < 1.3.8.0 bound.

@andreasabel
Copy link
Member Author

andreasabel commented Nov 20, 2022

Serokell hackage search for import safe System.Directory only finds MissingH. Fringe use indeed, and I only started the import safe thing because HLS suggested it (didn't even know about this syntax).

... MIssingH to remove Safe pragmas.

Will do, seems the most pragmatic course of action.

@andreasabel
Copy link
Member Author

Serokell hackage search for import safe System.Directory only finds MissingH.

Actually, I overlooked lio-fs which is a (likely academic) security library so they crucially rely on SafeHaskell. I revised their packages to directory < 1.3.8:

In my case (MissingH) the safe imports were added (Feb 2022) by the HLS tactic that creates explicit import lists (a terribly useful tool otherwise, maybe this is a flaw).

I might have been unjust in my assessment here because I misremembered. HLS only adds safe to import when the importing module is declared Safe (see my relevant commit to MissingH). So, my apologies to the HLS developers, their logic is correct.

@mpickering
Copy link
Contributor

This would mean filepath itself also needs a major version bump. This is what I initially planned to do, but GHC team said that will create more work for them.

My understanding is that System.OsPath is a new module, so that a minor bump is still fine for filepath.

The issue here seems to be that System.Directory (an existing module) changes its safety status, I agree with @Bodigrim that removing the use of SafeHaskell is the best thing to do..

Thanks everyone for the discussion about this.

@andreasabel
Copy link
Member Author

For package MissingH, I am dropping LANGUAGE Safe for all modules importing System.Directory in this PR:

@Rufflewind
Copy link
Member

Are there any outstanding issues that remain here?

If not I will close this. Thank you all for the discussions.

ivanperez-keera added a commit to Copilot-Language/copilot that referenced this issue Jan 8, 2024
**Description**

`copilot-theorem` is failing to compile with GHC 9.6.3. This is because
the module `Control.Monad.State` no longer re-exports `Control.Monad`,
so the functions `ap`, `forM`, `when`, `liftM` and `liftM2` are not in
scope. Another issue is that `System.Directory` can no longer be
imported safely. These errors are causing the build to fail on hackage,
which uses GHC 9.6.3.

**Type**

- Bug: Failure to compile with version of dependency.

**Additional context**

- haskell/directory#147

**Requester**

- Ivan Perez

**Method to check presence of bug**

The following Dockerfile checks whether Copilot can be installed with
GHC 9.6, in which case it prints the message Success:

```Dockerfile
FROM ubuntu:focal

RUN apt-get update

RUN apt-get install --yes libz-dev
RUN apt-get install --yes git

RUN apt-get install --yes wget
RUN mkdir -p $HOME/.ghcup/bin
RUN wget https://downloads.haskell.org/~ghcup/0.1.17.7/x86_64-linux-ghcup-0.1.17.7 -O $HOME/.ghcup/bin/ghcup

RUN chmod a+x $HOME/.ghcup/bin/ghcup
ENV PATH=$PATH:/root/.ghcup/bin/
ENV PATH=$PATH:/root/.cabal/bin/
RUN apt-get install --yes curl
RUN apt-get install --yes gcc g++ make libgmp3-dev

RUN ghcup install ghc 9.6.3
RUN ghcup install cabal 3.4
RUN ghcup set ghc 9.6.3
RUN cabal update

SHELL ["/bin/bash", "-c"]
CMD git clone $REPO \
    && cd $NAME \
    && git checkout $COMMIT \
    && cabal install --lib copilot**/ \
    && echo Success
```

Command (substitute variables based on new path after merge):
```
$ docker run -e "REPO=https://github.com/Copilot-Language/copilot" -e "NAME=copilot" -e "COMMIT=<HASH>" -it copilot-verify-491
```

**Expected result**

Compiling with GHC 9.6 succeeds. Running the above image prints the
message Success, indicating that Copilot can be compile with GHC 9.6.3.

**Solution implemented**

Modify `copilot-theorem` to import `Control.Monad` explicitly, instead of via
re-exports from other modules.

Modify the module that imports `System.Directory` so that it is marked
as trustworthy instead of Safe.

**Further notes**

None.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: b-discussion This is a discussion with unclear objectives.
Projects
None yet
Development

No branches or pull requests

5 participants