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

Add a new rule haskell_prebuilt_library. #337

Merged
merged 2 commits into from
Jul 18, 2018
Merged

Add a new rule haskell_prebuilt_library. #337

merged 2 commits into from
Jul 18, 2018

Conversation

judah
Copy link
Collaborator

@judah judah commented Jul 16, 2018

This rule forwards a prebuilt dependency, so that it may be
specified directly in deps rather than prebuilt_dependencies.

This gives progress on #75. Similarly, it simplifies the use of Hazel,
which can now generate repositories for the prebuilt dependencies
just like for regular packages.

Relevant Hazel change that would depend on this one: FormationAI/hazel@143d8f7#diff-c40927b6657800717d4b0b498e1bad55

Let me know what you think of this change. I'm open to modifying the API/names.

@judah judah requested review from mboes and mrkkrp July 16, 2018 02:59
Copy link
Member

@mrkkrp mrkkrp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

static_libraries = acc.static_libraries,
dynamic_libraries = acc.dynamic_libraries,
interface_files = acc.interface_files,
prebuilt_dependencies = set.insert(acc.prebuilt_dependencies, pkg),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use set.mutable_insert here.

Copy link
Member

@mboes mboes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very nice point in the design space. Thanks for contributing it. I think it works better than having a separate prebuilt_dependencies field.

haskell_prebuilt_library has (pretty much) the same function as cc_import in the cc_* rules world. Would it make sense to name it haskell_import_library? prebuilt_dependencies will soon become deprecated anyways, so we don't need to worry about being consistent with that.

Example:
```bzl
haskell_prebuilt_library(
name = "base_pkg",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be typo in this example, since you name the target "base_pkg" by the rule below depends on ":base".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

haskell_prebuilt_library = rule(
_haskell_prebuilt_library,
attrs = dict(
package = attr.string(doc = "A non-Bazel-supplied GHC package name."),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good for the package to have name as the default value. In fact, could name alone be sufficient? The list of allowed characters in a package is a subset of the legal target names. Although people might want to import ghc-prim as ghc_prim.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion; I've set it default to name. (I agree that ghc-prim vs ghc_prim means we should keep the separate attribute as an option.)

@mboes
Copy link
Member

mboes commented Jul 17, 2018

The CI failures seem to be due to a (transient?) bug in the CircleCI infrastructure.

@judah
Copy link
Collaborator Author

judah commented Jul 17, 2018

Renamed to haskell_import_library; PTAL.

judah added a commit to FormationAI/hazel that referenced this pull request Jul 18, 2018
Uses the functionality from tweag/rules_haskell#337.

Hazel now generates repository rules for "core" GHC packages like `base`
and `bytestring`.  Thus, rules that uses Hazel shouldn't need to specify
the `prebuilt_dependencies` attribute anymore.

Also change all references to "prebuilt" to "core packages", to match the
terminology in stack build plans.
judah added 2 commits July 18, 2018 11:00
This rule forwards a prebuilt dependency, so that it may be
specified directly in `deps` rather than `prebuilt_dependencies`.

This gives progress on #75.  Similarly, it simplifies the use of Hazel,
which can now generate repositories for the prebuilt dependencies
just like for regular packages.
@mboes mboes force-pushed the prebuilt-library branch from ab0378e to 0661c8e Compare July 18, 2018 09:01
@mboes
Copy link
Member

mboes commented Jul 18, 2018

@judah FYI I know I proposed haskell_import_library earlier, but that's because I didn't remember that in the CC rules this is actually called cc_import (no _library). So just pushed a commit to have haskell_import` here too. Holler if you disagree and we can revert.

@mboes mboes merged commit 2499b7c into master Jul 18, 2018
@mboes mboes deleted the prebuilt-library branch July 18, 2018 09:19
mboes added a commit that referenced this pull request Jul 18, 2018
Each PR individually was passing tests, but the combination was not.
judah added a commit to FormationAI/hazel that referenced this pull request Jul 20, 2018
Uses the functionality from tweag/rules_haskell#337.

Hazel now generates repository rules for "core" GHC packages like `base`
and `bytestring`.  Thus, rules that uses Hazel shouldn't need to specify
the `prebuilt_dependencies` attribute anymore.

Also change all references to "prebuilt" to "core packages", to match the
terminology in stack build plans.
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.

3 participants