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.

Sign up for GitHub

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

Jump to bottom

Improve documentation of build_file and add tests #35

Merged
merged 1 commit into from
Oct 25, 2018
Merged

Improve documentation of build_file and add tests #35

merged 1 commit into from
Oct 25, 2018

Conversation

Profpatsch
Copy link
Contributor

The BUILD file in the repository created by nixpkgs_package had
three filegroups that exposed bin, include and lib, but none
that exposed all files of the nix build output.
We introduce a new target named :output that does just that.

fixes #9

@Profpatsch Profpatsch requested review from Fuuzetsu and mboes October 24, 2018 13:00
mboes
mboes reviewed Oct 24, 2018
View reviewed changes
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.

I'm not sure we want this by default. The Bazel manual warns against blanket depending on everything (including in bold here, see also here), because it means Bazel will have to check more files at each build to see what it needs to rebuild. It's also a very blunt tool, when typically only a few select files really need to be part of the dependency graph.

What's your use case for depending on everything in a Nix package?

@Profpatsch Profpatsch force-pushed the expose-all-files branch 6 times, most recently from 8cb53e3 to af0c922 Compare October 24, 2018 15:36
@Profpatsch Profpatsch changed the title Expose all files in a filgroup target named :output Improve documentation of build_file and add tests Oct 24, 2018
@Profpatsch
Copy link
Contributor Author

Point taken, I’ve instead opted to improve the user documentation for build_file and add two tests.

@Profpatsch Profpatsch mentioned this pull request Oct 24, 2018
Merged
mboes
mboes approved these changes Oct 24, 2018
View reviewed changes
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.

LGTM.

@Profpatsch
Copy link
Contributor Author

CircleCI is red, #36 needs to be merged before it prints the error logs so I can debug this.

@Fuuzetsu
Copy link
Collaborator

Fuuzetsu commented Oct 25, 2018 •
edited
Loading

I guess I'm happy with #9 being closed because it's documented in this PR how to access the other files.

@Profpatsch
Improve documentation of build_file and add tests
4a75b2d 
Adds some user documentation on how to generate bazel targets for the
repositories generated by `nixpkgs_package`.

Also adds two tests to check whether this actually works as
documented.
@Profpatsch
Copy link
Contributor Author

Nice, had some bashisms in the test_output script, but green now.

@Profpatsch Profpatsch merged commit 7c35724 into master Oct 25, 2018
@Profpatsch Profpatsch deleted the expose-all-files branch January 14, 2019 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mboes mboes mboes approved these changes

@Fuuzetsu Fuuzetsu Awaiting requested review from Fuuzetsu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider exposing all files in a filegroup
3 participants
@Profpatsch @Fuuzetsu @mboes