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

shell.nix: Add buildable flag #2828

Merged
merged 1 commit into from
Sep 9, 2020
Merged

shell.nix: Add buildable flag #2828

merged 1 commit into from
Sep 9, 2020

Conversation

haslersn
Copy link
Contributor

Please ask @poelzi before merging!

If buildable = true, then we don't include convenient development tools
in the nativeBuildInputs. Most notable, ccache which would prevent the
derivation from being buildable will not be included and thus shell.nix
can be built using

$ nix build -f shell.nix --arg buildable true
or:
$ nix-build shell.nix --arg buildable true

@haslersn haslersn mentioned this pull request May 28, 2020
Merged
@matthiasbeyer
Copy link
Contributor

If everyone is fine with an additional default.nix in the project root, going down the "default+shell file"-path would be way cleaner though.

But if this works and more .nix files are discouraged, I'd ACK this!

@Holzhaus Holzhaus added the build label May 29, 2020
@haslersn haslersn force-pushed the enhancement/nix-shell-buildable branch from 792517f to 0a76ace Compare June 1, 2020 19:40
@matthiasbeyer
Copy link
Contributor

I'm a bit unsure about the naming here. Is "buildable" really the best name here? Because ... with the development tools included, mixxx is still buildable, right?

What about using "development" as a parameter name and having it by default set to true?

@haslersn
Copy link
Contributor Author

haslersn commented Jun 1, 2020

buildable because you can build the derivation using Nix. We could alternatively call it nix-buildable, or just have a separate default.nix.

@haslersn haslersn force-pushed the enhancement/nix-shell-buildable branch from 0a76ace to d1f6f36 Compare June 1, 2020 20:04
@haslersn
Copy link
Contributor Author

haslersn commented Jun 3, 2020

@poelzi do you like to look at this?

@Holzhaus
Copy link
Member

Holzhaus commented Jun 3, 2020

Does this file support comments? If so, could you add a comment explaining what it's used for at the top of the file? I have no idea :D

@haslersn
Copy link
Contributor Author

haslersn commented Jun 3, 2020

That's something I can do regardless of whether this PR gets merged or not. So I will do a separate PR.

@uklotzde
Copy link
Contributor

@poelzi Any comments on this?

@Holzhaus
Copy link
Member

@poelzi ping

@Holzhaus Holzhaus marked this pull request as draft June 21, 2020 23:27
@poelzi
Copy link
Contributor

poelzi commented Jul 28, 2020

how about the naming it 'releaseMode' with default to false ?
I personally like working defaults without having to read into a nix file to understand which parameters are required for getting a derived build or a development shell. But I can understand that cluttering the project root is also discouraged.
Is there no variable to test if the file was opened by nix-shell or nix-build ?

@poelzi
Copy link
Contributor

poelzi commented Jul 28, 2020

with the rename to releaseMode, LGTM

@poelzi
Copy link
Contributor

poelzi commented Aug 31, 2020

ping @haslersn

@haslersn
shell.nix: Add releaseMode flag
020b718 
If `releaseMode = true`, then we don't include convenient development tools
in the `nativeBuildInputs`. Most notable, `ccache` which would prevent the
derivation from being buildable will not be included and thus `shell.nix`
can be built using

```
$ nix build -f shell.nix --arg releaseMode true
or:
$ nix-build shell.nix --arg releaseMode true
```
@haslersn haslersn force-pushed the enhancement/nix-shell-buildable branch from d1f6f36 to 020b718 Compare August 31, 2020 21:56
@Be-ing Be-ing marked this pull request as ready for review August 31, 2020 21:58
@Be-ing
Copy link
Contributor

Be-ing commented Aug 31, 2020

@poelzi can you check the latest commit before we merge?

@Holzhaus
Copy link
Member

Holzhaus commented Sep 9, 2020

@poelzi ping

poelzi
poelzi approved these changes Sep 9, 2020
View reviewed changes
Copy link
Contributor

@poelzi poelzi left a comment

Choose a reason for hiding this comment

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

LGTM

@Be-ing Be-ing merged commit 31f86cf into mixxxdj:master Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poelzi poelzi poelzi approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@haslersn @matthiasbeyer @Holzhaus @uklotzde @poelzi @Be-ing