-
Notifications
You must be signed in to change notification settings - Fork 87
cabal: Add support for empty libraries #2287
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
base: master
Are you sure you want to change the base?
Conversation
Thank you, that looks good so far. It would be nice if this would be usable from With this in place we should be able to remove the workaround for empty packages here, right? Could you also add a test case (I was using the This is related to #1458, #1302 and (partially) also #2274 and #2200. For the last one, we would need to pass through the This gets a bit tedious over time, especially since Bazel's attr is a bit limited and does not support arbitrary values in dicts, just strings. We would have to add I was thinking about adding a rule which mimics the cabal attributes and returns a Provider that can be used to pass additional information to the haskell_cabal_library rule. Something along the lines of:
One could then pass any additional args to the
... which would generate WDYT? |
Sorry for the delay. I'll pick this back up again this week. |
I have a first pass at using a provider to handle this. I only handled empty libraries with a provider. Let me know if this along the lines that you were thinking. With the provider, it was easier to see how to handle this for a stack snapshot. I added to the component parsing with This allowed me to remove the longstanding empty package blacklist and just add those packages to the list of known defaults for stack snapshots. |
cd7f81a
to
7302733
Compare
Drop the check for `libraries != []` in the check for processing the package config file. Haskell supports empty libraries. In order for Bazel to support them, we need to process the package config, even for empty libraries.
7302733
to
7d3ffdc
Compare
Was hoping to get some progress on this. This fixes one of the biggest weaknesses that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pushing this forward!
Could you add a test case which utilizes the is_empty
cabal arg, please?
Also, it would be nice to wire this up in the module extension, so it can be used from a MODULE.bazel
file.
Also, some of the test cases currently fail with:
cabal_args': BUILD file not found in directory 'tools/cabal_args' of external repository @rules_haskell. Add a BUILD file to a directory to mark it as a package. and referenced by '@stackage//:bytestring-builder'
7d3ffdc
to
3e401b9
Compare
Sorry, this was missed from one of the commits. It's now added. One thing down, several more to go. |
I added a commit that I think does the right thing. I'll try and test it with modules. I hadn't yet converted over because ghc from nixpkgs wasn't working with modules for a while. I'll double check and try and switch over. |
Rather than continually proliferate additional settings on haskell_cabal_library, create a rule and provider that can hold cabal specific settings. Add `empty_lib` as the first such setting. When building an empty lib, bazel will look for the package file, instead of for any object files. This will fix several packages that are currently blacklisted. It will also allow the use of packages with sublibraries and an empty main lib. Currently there is no way to use this with a stack snapshot, short of vendoring a package and adding a `haskell_cabal_args` to the vendored build file. A simpler way is planned.
7fdb3bc
to
20cb919
Compare
Empty haskell libraries have been a longstanding problem. Add support for an additional component type `empty_lib`, which is like `lib`, except that an additional `haskell_cabal_args` target will be created for the library, instructing Bazel not to look for any object files. Remove the longstanding package blacklist for empty packages. Replace it with the now possible correct entries for those libraries, indicating that the main library is empty.
It can occur that a module has a source file named "Setup.hs" that isn't a cabal setup module. The current setup finder will find such a module and then setup will break. Add a flag to cabal_args to indicate that any such module should be ignored.
Modules require a facade for stack_snapshot. Push components_args through the facade and back into the main implementation.
20cb919
to
5ff9fd9
Compare
Can I get access to the buildbuddy secret so that my CI runs will be cached? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I get access to the buildbuddy secret so that my CI runs will be cached?
A workflow run from a forked PR does not have access to the secrets... I would have to make the secret public I guess. 🤔
default = False, | ||
doc = "Configuring a package makes it visible by default, unless `hidden` is set to True", | ||
), | ||
"components_args": attr.string_dict( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a TODO that we want to use attr.string_keyed_label_dict(providers = [HaskellCabalLibraryArgs])
here in later bazel versions?
"verbose": attr.bool(default = False), | ||
"custom_toolchain_libraries": attr.string_list(default = []), | ||
"enable_custom_toolchain_libraries": attr.bool(default = False), | ||
"components_args": attr.string_dict(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, could we use attr.label_keyed_string_dict(providers = [HaskellCabalLibraryArgs])
here instead, with a TODO to switch to string_keyed_label_dict
some time later.
This makes it explicit what to expect here.
Just call _invert
before passing its value to the _stack_snapshot
rule.
@@ -0,0 +1,13 @@ | |||
load("@rules_haskell//haskell:cabal.bzl", "haskell_cabal_args") | |||
|
|||
haskell_cabal_args( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this file should live here. I would prefer if it would be in haskell/cabal
instead. WDYT?
No, I could move my PR to a branch in this repository. I would just need to be a contributor. But it isn't urgent. |
Previously, an empty library that only exported modules from another library failed to build. Bazel would error out when the .a file wasn't created. These libraries are becoming more common in the Haskell ecosystem, so we need to support them.
There is a straightforward way to handle these libraries. We make the package database the default output. We avoid creating an output file for any libraries if the library is marked as empty. This is all straightforward bazel dependency/output management.
Adjust the cabal wrapper to fix up the package database even in the case that there is no library. Otherwise we end up with an empty package database, which isn't what we're looking for.