Opened 13 months ago

Last modified 13 months ago

#15541 new bug

package environment files and the GHC API

Reported by: lspitzner Owned by:
Priority: highest Milestone: 8.6.1
Component: GHC API Version: 8.4.3
Keywords: package environment Cc: alanz, dcoutts, osa1
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Documentation bug Test Case:
Blocked By: Blocking:
Related Tickets: #15513 Differential Rev(s):
Wiki Page:

Description

The GHC API respects package environment files, which is not documented and was not announced.

I consider this to be at least a severe documentation bug.

Afaik this goes back to ghc-8.0.

#15513 is a ticket that essentially asks for the corresponding entry in the migration guide.

Change History (9)

comment:1 Changed 13 months ago by alanz

Cc: alanz added

comment:3 Changed 13 months ago by lspitzner

The haddock on setSessionDynFlags currently reads:

Updates both the interactive and program DynFlags in a Session. This also reads the package database (unless it has already been read), and prepares the compilers knowledge about packages. It can be called again to load new packages: just add new package flags to (packageFlags dflags).

Returns a list of new packages that may need to be linked in using the dynamic linker (see linkPackages) as a result of new package flags. If you are not doing linking or doing static linking, you can ignore the list of packages returned.

I assume this is where package environment files are read.

So in addition to nothing being documented, this semantic change was made to a function with the innocuous name of setSessionDynFlags.

Please don't do that.

comment:4 Changed 13 months ago by bgamari

The motivation for this design is that package environments should be transparent to the user, giving tooling a means of configuring GHC for a project without further user intervention. This is much the same way GHC will automatically look at the environment's global and user package databases. That being said, package environments are often much more transient and it's not hard to see how this might go wrong. I see a few ways forward here:

  • Rip the interpretPackageEnv call out of setSessionDynFlags (or rather, initPackages) and require API users to do this manually.
  • Add a flag to setSessionDynFlags specifying whether package environments should be respected
  • Keep the status quo but document it

lspitzner, perhaps you have an opinion on what this interface should look like?

comment:5 Changed 13 months ago by bgamari

Cc: dcoutts added

comment:6 Changed 13 months ago by lspitzner

The motivation for this design is that package environments should be transparent to the user, giving tooling a means of configuring GHC for a project without further user intervention.

I consider the whole feature to be a mistake, see this summary post.

As a consequence, if I had to choose between the options you presented, I'd vote for the first (rip out this side-effect from setSessionDynFlags).

Some further opinionated thoughts:

  • If you expose getFoo and setFoo, getFoo >>= setFoo must behave as identity, free of (side- / other) effects. No amount of API docs excuses a violation.
  • It is likely to surprise and generally inadvisable to have any actions exposed in an API depend on CWD, especially by default.
  • The actions in the API should all have a single, clearly defined purpose.

In brittany we encountered exactly the same question when it came to user config handling in its API. Perhaps have a look at how the brittany API is designed when it comes to Config values: https://hackage.haskell.org/package/brittany-0.11.0.0/docs/Language-Haskell-Brittany.html. Note how parsePrintModule is explicit about its inputs, promises to be semantically pure, and that there are separate actions that explicitly handle the filesystem reading of config files.

comment:7 Changed 13 months ago by mpickering

Priority: highhighest

These issues with package environment files are quite serious so I am bumping the priority

comment:8 Changed 13 months ago by osa1

Cc: osa1 added

comment:9 Changed 13 months ago by lspitzner

It is unfortunate and unintended that the scope of these ticket(s) got progressively larger. This was a consequence of my thought-process (1. this is cabal misfeature 2. I can at least fix one usecase if only the API was documented 3. no wait, this transcends my usecase and should be fixed for both ghc UI and API). Sorry about that.

So while I'd like to have a ticket "turn package-environment feature off by default" for both UI and API, I don't want to create yet another ticket. I trust you can handle this appropriately.

Note: See TracTickets for help on using tickets.