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

waf: Added support for optional extra tools #63042

Merged
merged 2 commits into from Jun 17, 2019

Conversation

Projects
None yet
3 participants
@xbreak
Copy link
Contributor

commented Jun 12, 2019

The list of tools withTools are included as extra tools when building waf.

Example:

mywaf = callPackage ../development/tools/build-managers/waf {
  python = python3;
  withTools = [ "doxygen" ];
};
Motivation for this change

waf carries many extra tools (under waflib/extras) that are not included by default. This PR enables users to create a waf version with a selection of these extra tools using withTools .

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions (CentOS 7.4)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.
  • Tested on local waf project

@veprbl

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Is there a reason not to include all the tools by default?

e.g. https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=waf

@xbreak

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Is there a reason not to include all the tools by default?

Only that there are 99 of them, with varying degree of maintenance. Tools are not loaded by default, so even if the quality may vary from tool to tool they shouldn't cause any problems unless a user have a custom tool with the same name.

One additional factor that would support all tools could be that if a custom waf is used, then a custom wafHook would also be needed which would be fine except I didn't manage to override attributes of wafHook so I had to go for a full copy into our overlay (this may simply be a lack of expertise on my part).

If the consensus is to include them all I'd be happy to update the PR. In this case, would it still be ok if I provide the default list of all extra tools in withTools?

@FRidh

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

See also #62592

@veprbl

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Is there a reason not to include all the tools by default?

Only that there are 99 of them, with varying degree of maintenance. Tools are not loaded by default, so even if the quality may vary from tool to tool they shouldn't cause any problems unless a user have a custom tool with the same name.

I've counted 103 and they occupy about 510 Kb. The numbers should not matter too much as they are not loaded at all times. This is a long list of names though, I'm not entirely sure we want to maintain it.

Also it needs to be pointed out that the current state of PR is not a no-op because it removes "compat15" tool that is supposed to be included by default:
https://gitlab.com/ita1024/waf/blob/36898e12affcb9f9bc61a4056377b2bf40f4d2d7/wscript#L110-111

One additional factor that would support all tools could be that if a custom waf is used, then a custom wafHook would also be needed which would be fine except I didn't manage to override attributes of wafHook so I had to go for a full copy into our overlay (this may simply be a lack of expertise on my part).

Looking at the definition of wafHook I could not immediately see the way to override waf without overriding waf globally. I guess we could wrap wafHook implementation with callPackage so that we would be able do wafHook.override { waf = mywaf; }.

If the consensus is to include them all I'd be happy to update the PR. In this case, would it still be ok if I provide the default list of all extra tools in withTools?

Yes, that would be fine.

@xbreak xbreak force-pushed the xbreak:waf-tools branch from 8d5b29e to 4f8e1b5 Jun 13, 2019

@xbreak

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

This is a long list of names though, I'm not entirely sure we want to maintain it.

Agreed. And I wouldn't be able to propose a subset of tools to include either (I only know of the ones we use).

Also it needs to be pointed out that the current state of PR is not a no-op because it removes "compat15" tool that is supposed to be included by default...

Thanks, I also just realized this. I've updated the commit to fix this.

Looking at the definition of wafHook I could not immediately see the way to override waf without overriding waf globally. I guess we could wrap wafHook implementation with callPackage so that we would be able do wafHook.override { waf = mywaf; }.

That would be nice. Is it that straightforward? (I haven't dug into what makeSetupHook does)

Bottom line: Do we want add and maintain the list of extras?

@veprbl

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Agreed. And I wouldn't be able to propose a subset of tools to include either (I only know of the ones we use).

Let's abandon this idea for now. Those can be added later if needed.

That would be nice. Is it that straightforward? (I haven't dug into what makeSetupHook does)

It should be very simple. Just copy the definition of wafHook from all-packages to a separate nix file in waf directory, add the function header (something like { makeSetupHook, waf }:) on top, use callPackage to include the definition from the nix file.

waf: Added support for optional extra tools
The list of tools `withTools` are included as extra tools when building
waf.

Example:

    mywaf = callPackage ../development/tools/build-managers/waf {
      python = python3;
      withTools = [ "doxygen" ];
    };

@xbreak xbreak force-pushed the xbreak:waf-tools branch from 4f8e1b5 to 5453a62 Jun 13, 2019

@veprbl

veprbl approved these changes Jun 13, 2019

@veprbl veprbl merged commit 2b51328 into NixOS:master Jun 17, 2019

9 of 11 checks passed

grahamcofborg-eval Calculating Changed Outputs
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@veprbl

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@xbreak Thank you!

@xbreak xbreak deleted the xbreak:waf-tools branch Jun 25, 2019

dmvianna added a commit to dmvianna/nixpkgs that referenced this pull request Jun 25, 2019

waf: Added support for optional extra tools (NixOS#63042)
The list of tools `withTools` are included as extra tools when building
waf.

Example:

    mywaf = callPackage ../development/tools/build-managers/waf {
      python = python3;
      withTools = [ "doxygen" ];
    };

@xbreak xbreak referenced this pull request Jul 3, 2019

Merged

wafHook: Refactored wafHook to use callPackage #64250

2 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.