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

haskell: invariant violation: no "data" output for enableSeparateDataOuput with custom outputs #35032

Closed
nmattia opened this issue Feb 16, 2018 · 9 comments

Comments

@nmattia
Copy link
Contributor

nmattia commented Feb 16, 2018

Issue description

If enableSeparateDataOutput is enabled, and if custom outputs are specified (which do not contain data) then the build fails.

Steps to reproduce

Create a haskell package with enableSeparateDataOutput = true; and outputs = ["out"]

Technical details

The generic builder assumes that enableSeparateDataOutput == true implies that $data is set. This is not guaranteed in case of custom outputs.

Found in current master.

@peti
Copy link
Member

peti commented Feb 16, 2018

The generic builder assumes that enableSeparateDataOutput == true implies that $data is set.

Yes, the generic builder assumes that and I see no reason why one might consider that a bug. If you have you no $data output, then you cannot enable enableSeparateDataOutput. What is the problem?

@zimbatm
Copy link
Member

zimbatm commented Feb 16, 2018

bug is probably a bit strong.

I think the question is whenever the caller of the function should be aware of the correlation or if enabling the flag should just add the "data" attribute to the outputs automatically. Depending on where both options are coming from it adds more coupling that necessary. #35040 proposes to add a bit more magic and append the "data" attribute automatically.

@nmattia
Copy link
Contributor Author

nmattia commented Feb 16, 2018

The problem is that it's bad UX: the behavior of enableSeparateDataOutput isn't documented and it's very easy to get the derivation in an inconsistent state, which takes a bit to debug. The only data I had when debugging this was mkdir -p: missing operand.

If you have you no $data output, then you cannot enable enableSeparateDataOutput.
This, especially, needs to either be made clear to the user (potentially in the documentation but especially in the error) or to be made impossible to not fulfill.

Here are some suggestions:

  1. Throw an error early if enableSeparateDataOutput is true but $data doesn't exist, with a message mentioning what's expected
  2. Insert $data in the outputs if enableSeparateDataOutput is set, regardless of whether outputs was customized or not
  3. Infer enableSeparateDataOutput from the outputs.

@peti I'll be happy to submit a PR if you think one of these makes sense, or if you have other suggestions.

@peti
Copy link
Member

peti commented Feb 16, 2018

I still don't understand this issue. The generic Haskell builder does not even accept any argument called outputs. The API does not allow to create a Haskell package with outputs = ["out"] set.

@zimbatm
Copy link
Member

zimbatm commented Feb 16, 2018

My bad, in our code overrideAttrs has been used to change the outputs attribute.

The generic build branches on args.outputs if it's provided: https://github.com/NixOS/nixpkgs/pull/35040/files#diff-360bed088faac7580cee137eeffb39e0R194

So I guess the other option would be to remove the branch and disallow the use-case. Or add outputs and an explicit argument.

@peti
Copy link
Member

peti commented Feb 16, 2018

I cleaned that code up a bit in 25ee251. Apparantly, that if-then-else conditional slipped in by accident when the multi-outputs PR was merged. It would be nice to guard against an inconsistent state, i.e. by means of an assertion. Not sure how to accomplish that in a way that's resistant against overrideAttrs, though.

@zimbatm
Copy link
Member

zimbatm commented Feb 16, 2018

In our case the package produces multiple binaries and it would be useful to retain the multiple outputs capability because they get used in different scenarios (multiple services, integration tests, DB migration tests, test logs).

I guess we could create more derivations from this one to split the outputs manually.

@nmattia
Copy link
Contributor Author

nmattia commented Feb 16, 2018

@peti thanks for 25ee251. I didn't realize we were bypassing the Haskell interface to override outputs.

@nmattia nmattia closed this as completed Feb 16, 2018
@peti
Copy link
Member

peti commented Feb 16, 2018

I don't know ... I didn't want to expose the outputs attribute to users of this code precisely because that opens up a can of worms. The builder is quite complex and full of assumptions that may or may not hold.

You can manipulate outputs in a somewhat controlled manner by providing your own mkDerivation function. It's just that when you do this, then you have a loaded shotgun pointed firmly at your foot and everyone assumes that you know why you are doing this. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants