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

juce: enable extras (includes Projucer) #288898

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

liarokapisv
Copy link
Contributor

@liarokapisv liarokapisv commented Feb 14, 2024

Description of changes

This commit adds support for the various extra Juce utilities that
are hidden behind the JUCE_BUILD_EXTRAS cmake configuration flag.
The most important of these is probably the Projucer program that is responsible for Juce project scaffolding.

There are no installation targets for these so we manually install them.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

juce = callPackage ../development/misc/juce {
juce = callPackage ../by-name/ju/juce/package.nix {
Copy link
Contributor

@kashw2 kashw2 Feb 16, 2024

Choose a reason for hiding this comment

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

As part of RFC0140 we no longer require a definition in all-packages.nix. We can remove the juce definition and declaration here.

To confirm this is working you should be able to run nix-build -A juce without issue.

Copy link
Contributor Author

@liarokapisv liarokapisv Feb 16, 2024

Choose a reason for hiding this comment

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

The issue is that we need to override the default stdenv so that it uses the 11.0 sdk in darwin. Should this be done within the package instead ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't see that we were overriding stdenv for darwin. This looks fine

@kashw2
Copy link
Contributor

kashw2 commented Feb 16, 2024

Addresses #252737 (comment)

@kashw2
Copy link
Contributor

kashw2 commented Feb 16, 2024

Result of nixpkgs-review pr 288898 run on x86_64-linux 1

1 package built:
  • juce

Copy link
Contributor

@kashw2 kashw2 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this

@kashw2
Copy link
Contributor

kashw2 commented Feb 17, 2024

Looks like this fails on:

aarch64-darwin
x86_64-darwin

@liarokapisv
Copy link
Contributor Author

I think it's probably different paths in the build folder. I will investigate.

@liarokapisv
Copy link
Contributor Author

liarokapisv commented Feb 17, 2024

@kashw2 Fixed. Had to add a few extra frameworks, handle ".app" paths and work around #19098. Checked on aarch64-darwin, but not x86_64-darwin.

Copy link
Contributor

@kashw2 kashw2 left a comment

Choose a reason for hiding this comment

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

Lgtm, awesome stuff

@liarokapisv
Copy link
Contributor Author

Any idea why the pkgs/by-name check is failing?

@NobbZ
Copy link
Contributor

NobbZ commented Feb 18, 2024

@grahamc do you know why ofBorgs by-name check fails with a file that isn't even touched in this PR?

pkgs.openobserve: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form pkgs.callPackage pkgs/by-name/op/openobserve/package.nix { ... } with a non-empty second argument.

while only juice is touched in the PR?

@ofborg ofborg bot requested a review from kashw2 February 18, 2024 15:01
@PowerUser64
Copy link
Contributor

PowerUser64 commented Mar 12, 2024

Tested building on a raspberry pi 4b and x86_64 (both NixOS), works just fine in both places from what I can tell! (UI is untested on arm, but I assume it works.) LGTM! Looks good, but I think there are some ways it can be made better. I've left some comments below.

appSuffix = isApp: if appCheck isApp then ".app" else "";
outDir = isApp: if appCheck isApp then "$out/Applications" else "$out/bin";
artifactPath = name: isApp: "extras/${name}/${name}_artefacts/Release/${name}${appSuffix isApp}";
mvArtefact = name: isApp: "mv ${artifactPath name isApp} ${outDir isApp}";
Copy link
Contributor

@PowerUser64 PowerUser64 Mar 12, 2024

Choose a reason for hiding this comment

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

Should this be mvArtifact? Artefact seems like a spelling error.
Edit: Turns out artefact is the British English spelling and artifact is the US English spelling. I see that it's spelled as artifact later on in this line (artifactPath). I think we should pick one or the other to go with rather than mixing and matching.

];

cmakeFlags = [
"-DJUCE_BUILD_EXTRAS=ON"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should have an override in inputs to disable this. It can be on by default (I think this is the only way it can get cached without creating an entry in all-packages.nix?), but it might not always be desirable to have this.

''
${lib.optionalString stdenv.isDarwin "mkdir $out/Applications"}

${mvArtefact "Projucer" true}
Copy link
Contributor

@PowerUser64 PowerUser64 Mar 12, 2024

Choose a reason for hiding this comment

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

I feel like there should be a desktop item for this, not sure if it typically exists on other distros. Thoughts?

(check out makeDesktopItem and copyDesktopItems if you want to try doing this)

Comment on lines 86 to 122
meta = with lib; {
description = "Cross-platform C++ application framework";
longDescription = "JUCE is an open-source cross-platform C++ application framework for desktop and mobile applications, including VST, VST3, AU, AUv3, RTAS and AAX audio plug-ins";
homepage = "https://github.com/juce-framework/JUCE";
license = with licenses; [ isc gpl3Plus ];
maintainers = with maintainers; [ liarokapisv kashw2 ];
platforms = platforms.all;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth adding mainProgram = Projucer in here.


stdenv.mkDerivation (finalAttrs: {
pname = "juce";
version = "7.0.9";
Copy link
Contributor

Choose a reason for hiding this comment

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

JUCE 7.0.10 is out, so this should probably be updated.

@liarokapisv
Copy link
Contributor Author

liarokapisv commented Mar 28, 2024

Should I perhaps avoid adding the other extra programs in the derivation and just include Projucer?
Also, I think instead of making the Projucer optional, it is instead better to add multiple-output derivation support.
Any thoughts?

@PowerUser64
Copy link
Contributor

PowerUser64 commented Mar 28, 2024

instead of making the Projucer optional, it is instead better to add multiple-output derivation support.

Do you mean adding an entry to all-packages.nix so there's a new package? (Like what bespoke synth does to have bespokesynth and bespokesynth-with-vst2 here.)

If so, I think that's a good idea. Projucer especially makes sense to have available as a separate package.

PS: while you're at it, it might be a good opportunity to rebase to master and fix the merge conflict.

@liarokapisv
Copy link
Contributor Author

I was actually referring to multi-output derivations but having a juce-tools package or similar like debian does would not be out of the question.

@PowerUser64
Copy link
Contributor

Ah yes, that would be very nice to have here as well. What do you think about having both a multi-derivation output "everything" package, as well as a smaller one that just has the JUCE library, for use in other packages? That way there's a package for if you want to do general JUCE development (desktop use), as well as one that is suitable for if you just need to compile something that uses JUCE but don't want to compile projucer and the kitchen sink.

@liarokapisv
Copy link
Contributor Author

I am not sure about it, I think it would be better to get more opinions on this. It may be worth adding the optional juce_extras attribute for now and iterate at a later time.

@PowerUser64
Copy link
Contributor

@grahamc do you know why ofBorgs by-name check fails with a file that isn't even touched in this PR?

pkgs.openobserve: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form pkgs.callPackage pkgs/by-name/op/openobserve/package.nix { ... } with a non-empty second argument.

while only juice is touched in the PR?

(#288898 (comment))

@NobbZ Seems that this is fixed now

This commit adds support for the various extra Juce utilities that
are hidden behind the `JUCE_BUILD_EXTRAS` cmake configuration flag.
There are no installation targets for these so we manually install them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants