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

Handling of toolchain libraries in stack_snapshot #1685

Merged
merged 6 commits into from
Jan 21, 2022

Conversation

ylecornec
Copy link
Member

This PR removes the hard coded list of toolchain libraries from the bazel.bzl file.

If the stack_snapshot is to be used by ghc, we will rely on stack to decide the toolchain libraries.
With a different compiler such as asterius, this list of toolchain libraries is configurable through the new toolchain_libraries attribute.

About the call to stack

We consider libraries without a location field in the result of stack ls dependencies json --global-hint --external to be part of the toolchain.

This happens in two cases:

  • If it is present in the generated stack.yaml (considered as local)
  • if it is known to stack (through this file) and not present in the snapshot.

So there are two kinds of mismatch to fix:

  1. toolchain libraries that stack does not know about should be added to the stack.yaml file.
  2. libraries that stack wrongly considers part of the toolchain should be added to a custom stack snapshot.

The toolchain_libraries attribute

If this attribute is set, it must contain all of the toolchain libraries.
So that we can know that a library is not in the toolchain and have an error message telling the user it should be added a custom stack snapshot.

The WORKSPACE file

To avoid writing the list of toolchain libraries explicitely, the asterius toolchain writes this list to the toolchain_libraries.bzl file so that it can be imported in the WORKSPACE file.

Since this is linux only and we cannot conditionally load this file, the toolchain_libraries workspace rule will create a file with a default None value.

@ylecornec ylecornec requested a review from aherrmann January 19, 2022 08:23
Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

Good work, thank you!

Thank you for the test and the changelog entry!

Only some minor comments. Feel free to merge when addressed.

WORKSPACE Outdated
Comment on lines 598 to 608
(
stack_snapshot(
name = "stackage_asterius",
local_snapshot = "@rules_haskell//tests/asterius/stack_toolchain_libraries:snapshot.yaml",
packages = [
"xhtml",
],
stack_snapshot_json = "@rules_haskell//tests/asterius/stack_toolchain_libraries:snapshot.json",
toolchain_libraries = toolchain_libraries,
) if is_linux else None
)
Copy link
Member

Choose a reason for hiding this comment

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

I think the outer parenthesis are not strictly necessary.

Comment on lines +14 to +21
drop-packages:
- Win32
Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth a comment as to why this is being dropped.

Comment on lines +16 to +23
drop-packages:
- Win32
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Comment on lines +2 to +10
drop-packages:
- Win32
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@ylecornec ylecornec force-pushed the ylecornec/cabal_toolchain_libs branch from 1334139 to 49b91ce Compare January 20, 2022 07:46
@ylecornec ylecornec added the merge-queue merge on green CI label Jan 21, 2022
@mergify mergify bot merged commit 156b091 into master Jan 21, 2022
@mergify mergify bot deleted the ylecornec/cabal_toolchain_libs branch January 21, 2022 17:25
@mergify mergify bot removed the merge-queue merge on green CI label Jan 21, 2022
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.

2 participants