Track object and interface files explicitly in Starlark

[aherrmann]

Infer module names from source file paths and explicitly track object and interface files in Starlark.

Motivation

Closes #1260

Since #383 rules_haskell uses ctx.actions.declare_directory for object and interface files generated by GHC instead of tracking these explicitly. The reason for this is that GHC produces these files under paths that depend on the module name rather than the source file name, which we cannot determine in Starlark in general.

However, as #1260 describes, we have found that outputs declared by ctx.actions.declare_directory cause issues on Windows with distributed caching. Occasionally we end up with empty intermediate outputs (interface dir/manifest files) in the remote cache. These cause build failures when later builds read these intermediate outputs from the remote cache, e.g. after a rules_haskell update. They also tend to put the contents of Bazel output path on affected CI nodes into an invalid state. All this makes these issues very disruptive as they cause unrelated builds to fail, i.e. they often put all of CI on hold, and require bazel clean --expunge on the nodes and overwriting the remote cache to fix. Tracking the object and interface files explicitly and declaring them using ctx.actions.declare_file resolves this issue.

Also, the implementation of coverage support in rules_haskell already assumes that module names can be inferred from source files and the modules attribute to haskell_doctest also requires file names to match module names.

Changes

This PR implements explicit mapping from source file names to module names and the corresponding object and interface files in Starlark. A source file's module name is assumed to be the longest suffix that forms a valid module name. E.g.

//pkg:Some/Module.hs            -->  Some.Module
//pkg:src/Some/Module.hs        -->  Some.Module
//pkg:Upper/src/Some/Module.hs  -->  Some.Module
//Pkg:Some/Module.hs            -->  Pkg.Some.Module

This is very similar to the strategy described in #1232 (comment).

The main module is determined using the following heuristics:

• If a source file name matches the main module name, then this is the main module. E.g. Main.hs
• Else, if a source file path yields no valid module name, then we assume it is the main module. E.g. exe.hs.
• Else, if the source file is the only source file of a binary target, then we assume it is the main module.

Users can override this using the main_file attribute.

Further Information

I tested this strategy on the rules_haskell repository and the daml repository. With the strategy described above nearly all Haskell targets worked without changes. In rules_haskell there was one case of a file name not matching its module name and one target that required a main_file attribute. In daml there were two targets where modules had been renamed but the file paths had not been updated to the new module hierarchy. So, hopefully this change would have minimal impact on users.

This change does not affect the Cabal rules (haskell_cabal_library|binary). These do not require tracking object files as the full compilation happens in a single build action. Their interface files are still tracked via declare_directory. We don't even know which source files to a haskell_cabal_* rule will be compiled, so we cannot predict the interface files. However, as everything happens in a single build action there are no intermediate build results, and we have never observed issues like #1260 with the Cabal rules.

Discussion

This is a breaking change and effectively rolls back #383, though this implementation is more flexible than the prior src_strip_prefix approach. As #375 points out GHC is very lenient about source file names and this PR introduces an additional restriction on top of what GHC would require. However, many Haskell developers assume that source file names and module names should match, and some tools, e.g. GHCi's :load command or doctest, make that assumption as well. I.e. it is unclear that being that lenient is actually a wanted features.

One component that did require more leniency, at least than src_strip_prefix, was Hazel. However, since we have the Cabal rules Hazel is no longer required to build external dependencies, and even if, the strategy implemented in this PR is more lenient than src_strip_prefix and may well work for most Hackage packages.

This change introduces additional complexity to infer module names from source file names. However, arguably, it is a simplification, as the declare_directory approach requires pushing logic into build actions that could otherwise be written in Starlark, e.g. generating the object manifest files or the list of exposed modules.

Also, this change makes the implementation of coverage support consistent with the rest of rules_haskell.

Possible Extensions

• The handling of import dirs (-i flags e.g. for doctest) could be made more precise. I.e. instead of using src_strip_prefix we could use the prefix until the longest valid module name of each source file.
• If required src_strip_prefix could be repurposed to strip prefixes that would otherwise be considered part of the longest valid module name suffix. E.g. to strip Foo in src/Foo/Bar/Baz.hs.
• should module paths be relative to the root or the package? #1232 (comment) describes a project structure where module names start at the workspace root rather than package root. This PR already supports this if the full path from the package root forms a valid module name. If that is not the case, we could introduce src_prefix similar to include_prefix in cc_library to allow users to prepend a module prefix. E.g. turn package/Foo/Bar.hs into Package.Foo.Bar.

[aherrmann]

An alternative strategy that came up during discussion is to not pass -odir/-hidir flags to GHC. In that case GHC will generate the object and interface files right next to their corresponding source files and the file paths of the artifacts will be derived from the source file paths rather than the module names. This would give us the benefits of predictable output filepaths so that we can track individual output files using ctx.actions.declare_file, while at the same time not requiring us to predict the module names based on the file paths.

I've experimented with this idea. It works in principle, however, I discovered the following issues with this approach that make it seem less favorable:

• GHC makes strict assumptions about the paths of .hi files. The path of a .hi file underneath import-dirs has to match the module name exactly. So, for libraries we need to strip path prefixes that don't belong to the module name, and the remainder of the source file path has to match the module name (at least for libraries). E.g. for //package:src/My/Module.hs the module has to be named My.Module and we need to make sure that the .hi file has the path My/Module.hi underneath a path that is written into the import-dirs field of the package configuration file. I.e. we either need to revive a form of src_strip_prefix (or a list form of it), or infer import-dirs values based on file paths, which has similar complexity to inferring module names from file paths.
• Errors due to module name mismatch don't appear at the current target, but only deferred at the importing target. E.g. if the module Some.Module exists in My/Module.hs, then GHC will produce My/Module.hi without error. But, a later target that depends on the first and imports Some.Module will fail to build with "module not found". I.e. such errors are dormant until the module is actually imported.
• We have to copy/symlink source files into the output directory within the compile action and delete them at the end of the compile action. GHC will write .o/.hi outputs right next to the source files. However, Bazel only allows outputs underneath the bin-dir (bazel-out/.../bin). I.e. we can either symlink/copy sources so that GHC writes to the correct location right away, or move the outputs after the fact. On Windows, we don't have sandboxed builds. To avoid collisions with other build actions we cannot have GHC write into the source tree, so we need to copy the source files into the output tree. Additionally, again because there is no sandboxing on Windows, we have to remove those sources at the end of the build actions. Otherwise, when building a second target that depends on the first, GHC would still see the source files in import paths and would recompile them instead of using the .o/.hi outputs (GHC always prefers sources). I.e. we cannot implement the source file linking/copying in Starlark (there is no way to remove action outputs), but have to inline it into the compile action itself.
• We have to patch include paths (-I) or copy/symlink header files (or similar) that might be #included into the build tree as well. Such includes are relative to the source file paths. If we move the sources into the output directory, then these paths become invalid.
• Similar to the above, template Haskell allows to access the current source file path. Moving the source files could introduce subtle bugs with code that makes use of this feature. Granted, that's probably not much code out there, but it's a risk.

[Profpatsch]

What’s the status of this PR, can it be reviewed? I’ve had it stagnate in my browser tabs, hoping somebody else would review it first :)

[aherrmann]

What’s the status of this PR, can it be reviewed? I’ve had it stagnate in my browser tabs, hoping somebody else would review it first :)

Yes, it can be reviewed and discussed on.

This has been incorporated in daml for about two months now. We haven't had these specific Windows issues since, so it seems to have fixed them.

This is a breaking change and a restriction on what kinds of Haskell rule invocations are valid. That's why I added a discussion section. There haven't been any replies regarding that discussion, yet.

[thufschmitt]

Regarding the restriction on file names, this is already the case when using ghcide (and possibly other tools), so it might actually be a good thing to enforce it at compile time to notice errors quickly

[facundominguez]

I need to avoid declare_directory so the various actions from haskell_module can dump outputs in a common directory together with outputs of the build actions of haskell_library and haskell_binary. GHC would not let us do it with a combination of -hidir and -i: https://gitlab.haskell.org/ghc/ghc/-/issues/20569

I'll move forward with the original approach of this PR, unless someone objects.

The shortcomings of the alternative to copy the source files are ugly enough that I don't think it is worth continuing the debate.

[aherrmann]

@facundominguez Thanks for picking this up! No objection.

[facundominguez]

I'm taking this as a signal that we might want to abandon src_strip_prefix in haskell_module too, and use instead a new optional attribute module_name, to be specified whenever the module name doesn't match what the heuristics in this PR would infer.

[aherrmann]

Yes, I'm happy to go that way.

One thing to keep in mind: For haskell_repl to work we'll still need to be able to generate -i flags that point to the Haskell source files of a given haskell_module (REPL support for haskell_module is not implemented, yet). The src_strip_prefix attribute is a simple way to achieve this, assuming that the remaining file path matches the module name (Main modules are not imported by other modules, so a mismatch is not an issue in that case). However, if you can construct a -i search path based on a module_name attribute, then that's good as well.

[facundominguez]

Please, could someone help me diagnose what the problem with the readthedocs job is?

[ylecornec]

Please, could someone help me diagnose what the problem with the readthedocs job is?

The docutils python package was updated to a version not compatible with sphinx, which broke a lot of readthedocs builds.

It was fixed on master by #1620.

[facundominguez]

Thanks @ylecornec!

[facundominguez]

All green!

[aherrmann]

@facundominguez Thank you for picking this up. This looks great!

I cannot approve, since I originally opened that PR. But, feel free to approve yourself and then add to merge-queue.