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

Make mariadb conform to multiple-outputs #8494

Closed
wants to merge 7 commits into from

Conversation

wkennington
Copy link
Contributor

This should also fix issues where mariadb is missing references to the data directory which was set to /not/a/real/path before. It also splits out the build time dependency on groonga into a separate package instead of building the bundled version.

@wkennington
Copy link
Contributor Author

@vcunat Let me know if this makes sense with your multiple outputs changes.

@wkennington wkennington changed the title Make mariadb conform Make mariadb conform to multiple-outputs Jun 25, 2015
@vcunat vcunat added 1.severity: mass-rebuild This PR causes a large number of packages to rebuild new-package labels Jun 25, 2015
@vcunat
Copy link
Member

vcunat commented Jun 25, 2015

  • Changing pkgs.(lib)mysql is a rather mass rebuild, so it should better be filed against staging (not master).
  • In WIP: splitting packages into multiple outputs #7701 it's best practice to put "dev" as the first output, because the first output is used when you just use mysql (the most typical usage is in build inputs). When you define libmysql = mysql.dev and then use libmysql.lib, it seems rather confusing and I wonder if it even works.
  • Using things like echo "$man" >> $out/nix-support/propagated-native-build-inputs is probably useless. AFAIK it won't make manuals propagate when you put it into systemPackages (I've got c2fff72 for that), and nix-env installs all output if you don't specify which.
  • check_references "$dev" "$out": I fear these long lists will become slightly inefficient and they're rather long. For future, it would be best to have a nix primitive for that, as nix needs to do the scanning anyway (somehow extend allowedReferences etc.). For now, I'd at least add -F to grep.
  • Some adjustments will probably be needed/useful when this gets into WIP: splitting packages into multiple outputs #7701, but that seems unavoidable.

@wkennington
Copy link
Contributor Author

  • Sure I'll do that if / when this gets merged
  • I'd rather avoid confusion between the package which contains binaries and the one which provides the includes / libs. Users will want to reference the mysql package for the binaries and man pages while the inputs should all use the library variant. Regular users shouldn't need to install the dev components.
  • I actually need to fix this so that the main output only consists of the out output in the outputs list which systemPackages uses
  • This should be built in but it is actually quite fast so not really an issue. We should standardize on the naming of outputs as well as what outputs are allowed to refer to each other. I think the list here represents the most sane allowances.


check_references "$out" "$doc"
check_references "$dev" "$doc"
check_references "$man" "$doc"
Copy link
Member

Choose a reason for hiding this comment

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

This is a pretty unscalable way of checking references. I mean, it's basically O(n^2) lines (where n = the number of outputs)... It's also inefficient because it will do lots of I/O that Nix does anyway later.

@edolstra
Copy link
Member

Maybe this should be done on the multiple-outputs branch?

Also, why libmysql instead of mysql.lib?

@wkennington
Copy link
Contributor Author

Sure we could do this in O(n) time based on inputs by passing in an array
of directories to check. This is not the same as the nix checks because we
are ensuring certain reference never happen, even if they don't create
cycles. All nix seems to check for at the moment is cycles. For example, I
don't want bin to ever depend on dev which does happen in some packages
accidentally. However, dev will never depend on bin so it wouldn't be a
cycle in the accidental case and then not detected. Like I mentioned
earlier, a lot of this stuff should be part of the stdenv and used as an
enablable option.

On Thu, Jun 25, 2015, 01:53 Eelco Dolstra [email protected] wrote:

Maybe this should be done on the multiple-outputs branch?

Also, why libmysql instead of mysql.lib?


Reply to this email directly or view it on GitHub
#8494 (comment).

@wkennington
Copy link
Contributor Author

As for the libmysql output, it makes it easier to deal with nulls in
buildinputs and allows you to differentiate the deivations of mysql /
libmysql in the future, if you end up with something like libpulseaudio.

On Thu, Jun 25, 2015, 02:00 William Kennington [email protected]
wrote:

Sure we could do this in O(n) time based on inputs by passing in an array
of directories to check. This is not the same as the nix checks because we
are ensuring certain reference never happen, even if they don't create
cycles. All nix seems to check for at the moment is cycles. For example, I
don't want bin to ever depend on dev which does happen in some packages
accidentally. However, dev will never depend on bin so it wouldn't be a
cycle in the accidental case and then not detected. Like I mentioned
earlier, a lot of this stuff should be part of the stdenv and used as an
enablable option.

On Thu, Jun 25, 2015, 01:53 Eelco Dolstra [email protected]
wrote:

Maybe this should be done on the multiple-outputs branch?

Also, why libmysql instead of mysql.lib?


Reply to this email directly or view it on GitHub
#8494 (comment).

@edolstra
Copy link
Member

No, it shouldn't be part of stdenv, it should be a Nix feature. It's scanning all outputs for references anyway so it might as well check for disallowed references. In fact it already does: allowedReferences can specify output names (e.g. allowedReferences = ["out"]). However, you can't specify allowed references per output yet.

@edolstra
Copy link
Member

Maybe something like:

allowedReferences.bin = ["data" "man"];
allowedReferences.data = [];

@edolstra
Copy link
Member

Though probably it's more effective to specify the few references that you want to guard against, e.g.

disallowedReferences.bin = ["dev"];

@vcunat
Copy link
Member

vcunat commented Jun 25, 2015

Much of this discussion seems general to multiple-output handling, it seems. The whole situation in there's rather complicated, I found when digging into it, and proper resolution is probably better not hurried before this release. Maybe pick the update at least to staging, if it's tested, and leave more complex changes after branching it off?

The release... is already unlikely to happen this month, but that belongs into another thread, I guess.

@wkennington
Copy link
Contributor Author

Well, the reason I did this in the first place is that it's the only way to
fix the mariadb situation for packages like amarok which embed the server
and break at application startup when they can't find the character sets
which are provided in the new $data directory.

On Thu, Jun 25, 2015, 07:06 Vladimír Čunát [email protected] wrote:

Much of this discussion seems general to multiple-output handling, it
seems. The whole situation in there's rather complicated, I found when
digging into it, and proper resolution is probably better not hurried
before this release. Maybe pick the update at least to staging, if it's
tested, and leave more complex changes after branching it off?

The release... is already unlikely to happen this month, but that belongs
into another thread, I guess.


Reply to this email directly or view it on GitHub
#8494 (comment).

@vcunat
Copy link
Member

vcunat commented Jun 25, 2015

I see, so in some form we do need this in the release...

@wkennington
Copy link
Contributor Author

@edolstra I think the whitelists are actually smaller as anything conforming to the gnu standard should have approximately the following outputs:

  • dev: All of the headers and pkgconfig files as well as the occasional bin/*-config scripts which lists cflags and libs. This should propagate $lib.
  • bin: All of the runnable binaries (/bin, /sbin, /libexec) but somestimes plugins / libraries only needed by the current package.
  • lib: All of the libraries needed for linking to external applications
  • data: Any data files which will be referenced by bin / dev / lib
  • man: Manpages
  • aux: Any other shared data / doc / info that doesn't need to be directly referenced by the application. Sample etc files can go here.

With the allowed references:

allowedReferences = {
  dev = [ "dev" "lib" "data" ];
  bin = [ "bin" "lib" "data" ];
  lib = [ "lib" "data" ];
  data = [ "data" ];
  man = [ ];
  aux = [ "bin" "dev" "lib" "data" ];
};

Then if we follow @vcunat suggestion of having dev be the first output, we could write our top-level as
mariadb = (callPackage <mariadb> { }) // { outputs = [ "bin" "man" ]; }
So that systemPackages and user environments install the binaries and man pages but callPackage picks up the dev derivation.

@wkennington
Copy link
Contributor Author

@vcunat @edolstra I can get rid of the postFixup reference checks for the release, as they are only sanity checks needed during the expression writing process to weed out extraneous references.

I can also cleanup the expression enough to match the structure in my post above, so that we don't need the libmysql line in top-level/all-packages.

@vcunat
Copy link
Member

vcunat commented Jun 25, 2015

I was typically leaving libraries and other data that's always needed in $out, because that's where --prefix is set, so unspecified things stay in there (to minimize potential for problems).

You're right that the structure is very similar for most packages; that's why the mentioned branch quite reliably splits stuff by auto-passing parameters to autotools-style configure, etc.

@wkennington
Copy link
Contributor Author

That seems somewhat okay but it would be preferable to understand what kinds of data libs / bin depend on so that it can be minimized and all other auxiliary data goes to the "doc" like directory which is not allowed to be depended on by the libs / bin.

I suppose out could serve the purpose I'm talking about but it might make more sense to call it "aux" or something along those lines and get rid of the out identifier altogether when doing multiple outputs so that it is more descriptive.

@vcunat
Copy link
Member

vcunat commented Jul 20, 2015

The general multiple-output rework certainly won't be in the upcoming release, so some kind of a fix for the problems might be good to get merged independently.

@vcunat vcunat added this to the 15.07 milestone Aug 2, 2015
@vcunat
Copy link
Member

vcunat commented Aug 2, 2015

@wkennington: do you still mean to work on this? It seems to be blocking several kde4 apps at least.

@wkennington
Copy link
Contributor Author

Yeah I do, I've been busy with other work.

On Sun, Aug 2, 2015, 01:13 Vladimír Čunát [email protected] wrote:

@wkennington https://github.com/wkennington: do you still mean to work
on this? It seems to be blocking several kde4 apps at least.


Reply to this email directly or view it on GitHub
#8494 (comment).

@pSub
Copy link
Member

pSub commented Aug 18, 2015

Any updates on this?

@wkennington
Copy link
Contributor Author

I'll need to rebase and at least comment out the code for checking
dependencies. I think it's worth keeping around until we have a nix
feature, just not enabled in the default build. Anyone updating the package
should build with it enabled though.

On Tue, Aug 18, 2015, 13:11 Pascal Wittmann [email protected]
wrote:

Any updates on this?


Reply to this email directly or view it on GitHub
#8494 (comment).

@sjau
Copy link

sjau commented Aug 31, 2015

So there is a way to make it work now?

@wkennington
Copy link
Contributor Author

I'll take a look at getting this cleaned up soon. I was working on writing some nix mkDerivation features to help with restrictions on multiple-outputs but we need this before that will hit nix in a stable release.

@domenkozar domenkozar modified the milestones: 16.03, 15.09 Sep 9, 2015
@domenkozar
Copy link
Member

Won't make it into 15.09

@akaWolf
Copy link
Member

akaWolf commented Sep 25, 2015

Still no progress. Nice.

@domenkozar
Copy link
Member

Please, no passive-aggressive comments, it doesn't help with motivation/interes.

@akaWolf
Copy link
Member

akaWolf commented Sep 25, 2015

@domenkozar I can't use some of programs few months already, since I switched to NixOS because of that bug. What will help with motivation/interest then?

@vcunat
Copy link
Member

vcunat commented Sep 25, 2015

Negative comments are unlikely to help. Investigating the issue yourself might. (You're probably better motivated to fix it than the rest of participants ;-)

@peti
Copy link
Member

peti commented Oct 10, 2015

@akaWolf,

What will help with motivation/interest then?

you could offer a 5,000 Dollar cash reward to anyone who remedies this issue successfully.

@pSub
Copy link
Member

pSub commented Nov 29, 2015

I would really like to see this PR merged. However it is hard for me to see what needs to be done (besides resolving conflicts). What is the status of those mkDerivation features and what else needs to be done?

@domenkozar
Copy link
Member

@vcunat is this part of closure-size branch?

@domenkozar domenkozar modified the milestones: 16.09, 16.03 Feb 29, 2016
@vcunat
Copy link
Member

vcunat commented Mar 1, 2016

It's not part of any branch AFAIK (in the official repository; maybe it is in triton).

@wkennington
Copy link
Contributor Author

Nope, im going to get back to this later.
On Tue, Mar 1, 2016 at 2:07 AM Vladimír Čunát [email protected]
wrote:

It's not part of any branch AFAIK (in the official repository; maybe in
triton).


Reply to this email directly or view it on GitHub
#8494 (comment).

@vcunat vcunat added the 2.status: work-in-progress This PR isn't done label Mar 1, 2016
@pwetzel pwetzel mentioned this pull request Jul 1, 2016
@vcunat
Copy link
Member

vcunat commented Aug 14, 2016

This seems dead and superseded by #17413.

@vcunat vcunat closed this Aug 14, 2016
@vcunat
Copy link
Member

vcunat commented Aug 14, 2016

Well, building everything in a single build could be better than the referenced PR, but I didn't want to mess with --basedir and similar problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.severity: mass-rebuild This PR causes a large number of packages to rebuild 2.status: work-in-progress This PR isn't done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants