-
-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
material-maker: init at 1.3 #331491
base: master
Are you sure you want to change the base?
material-maker: init at 1.3 #331491
Conversation
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/package-godot-projects-to-nixpkgs/49923/15 |
Its running checks for darwin, I've not package it for darwin because i doesnt have a mac and it would be pointless as they could install it via a .dmg. Should i try to add darwin compatibity or maybe enable something so it doesnt bother about it ? |
If it can be done, I don't see why we shouldn't package this for mac as well. We should refactor the current derivation first, though, then we'll look into doing that. I'm currently in the process if reviewing this, so you can expect my suggestions shortly. By the way, perhaps it's best if you mark this PR as draft, since it's not ready to be merged. |
meta = with lib; { | ||
homepage = "https://www.materialmaker.org/"; | ||
description = "Tool based on Godot Engine that can be used to create textures procedurally and paint 3D models."; | ||
license = licenses.mit; | ||
platforms = [ "x86_64-linux" ]; | ||
maintainers = with maintainers; [ aurreland ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meta = with lib; { | |
homepage = "https://www.materialmaker.org/"; | |
description = "Tool based on Godot Engine that can be used to create textures procedurally and paint 3D models."; | |
license = licenses.mit; | |
platforms = [ "x86_64-linux" ]; | |
maintainers = with maintainers; [ aurreland ]; | |
meta = { | |
homepage = "https://www.materialmaker.org/"; | |
description = "Tool based on Godot Engine that can be used to create textures procedurally and paint 3D models."; | |
license = lib.licenses.mit; | |
platforms = lib.platforms.x86_64-linux; | |
maintainers = with lib.maintainers; [ aurreland ]; |
See #208242
pname = "material-maker"; | ||
version = "1.3"; | ||
|
||
src = if stdenv.isLinux then fetchurl { | ||
url = "https://github.com/RodZill4/material-maker/releases/download/1.3/material_maker_1_3_linux.tar.gz"; | ||
hash = "sha256-Y8+ZwXy3zqnsxqqaZeVgFSxLzmUkq+rBzbq8tEDc8/g="; | ||
} else throw "unsupported platform"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use fetchzip
instead of fetchurl
since it auto-unpacks. We can also reuse the defined attributes like pname
, version
and versionUnderscore
pname = "material-maker"; | |
version = "1.3"; | |
src = if stdenv.isLinux then fetchurl { | |
url = "https://github.com/RodZill4/material-maker/releases/download/1.3/material_maker_1_3_linux.tar.gz"; | |
hash = "sha256-Y8+ZwXy3zqnsxqqaZeVgFSxLzmUkq+rBzbq8tEDc8/g="; | |
} else throw "unsupported platform"; | |
pname = "material-maker"; | |
inherit version; | |
src = | |
if stdenv.isLinux then | |
fetchzip { | |
url = "https://github.com/RodZill4/${pname}/releases/download/${version}/material_maker_${versionUnderscore}_linux.tar.gz"; | |
hash = "sha256-WEu5gVfnswB5zYzu3leOL+hKOBzJbn48gHQKshlfOh4="; | |
} | |
else | |
throw "unsupported platform"; |
As a tip, you can batch commit multiple suggestions from the See Incorporating feedback in your pull request - GitHub Docs |
Thanks for the huge work eljamm and d-brasher, i will look into how we could add support for darwin. |
Since we're using
|
Good to know will change to this |
Just as a plus, since we're inheriting |
Should i pass it to ready now ? |
Not yet, we still need to add darwin support. |
How could we add support for darwin, because we are using binary do they work out of the box ? or should we try to remake the package to be compiled like lorien does ? |
If the platform is Darwin, we just copy the source to the Applications folder, as far as I know, at least. |
I will try tomorrow on an old mac i got, but i won't be able to do much till 24/08 as i am going on a vacation. |
Well, in that case you can open this PR for review because I think this is good enough to be merged. If we can figure out how to package and test this on mac, then we'll include it here tomorrow, else we can add that in a separate PR if this gets merged first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think it could be a good thing to squash the commits in order to have just See https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions |
I totally forgot about that 🤦 Thanks! |
It now appears to miss a (if it can be of help, to squash commits I usually use |
'' | ||
runHook preInstall | ||
'' | ||
+ lib.optionalString stdenvNoCC.isLinux '' | ||
install -D -m 755 $src/material_maker.x86_64 $out/opt/material-maker | ||
install -D -m 644 $src/material_maker.pck $out/opt/material-maker.pck | ||
|
||
for dir in library nodes environments meshes export; do | ||
cp -R $src/$dir $out/opt | ||
done | ||
|
||
cp -R $src/examples $examples | ||
|
||
mkdir -p $out/share/ | ||
cp -R $src/doc $out/share | ||
|
||
install -D -m 644 $src/doc/_static/icon.png -t $out/share/icons/hicolor/256x256/apps/ | ||
|
||
mkdir -p $out/bin | ||
ln -s $out/opt/material-maker $out/bin/material-maker | ||
'' | ||
+ lib.optionalString (builtins.elem "doc" finalAttrs.meta.outputsToInstall && stdenvNoCC.isLinux) '' | ||
ln -s $doc/share/doc $out/opt/doc | ||
'' | ||
+ | ||
lib.optionalString (builtins.elem "examples" finalAttrs.meta.outputsToInstall && stdenvNoCC.isLinux) | ||
'' | ||
ln -s $examples $out/opt/examples | ||
'' | ||
+ lib.optionalString stdenvNoCC.isDarwin '' | ||
mkdir -p $out/Applications | ||
find ./ -type d -name "*.app" -exec cp -r {} /destination/directory $out/Applications \; | ||
'' | ||
+ '' | ||
runHook postInstall | ||
''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, looping over getAllOutputNames
is more readable.
Also, since the command that installs the icon also creates the $out/share
directory, we can move cp -R $src/doc $out/share
below that so we don't have to mkdir -p $out/share/
'' | |
runHook preInstall | |
'' | |
+ lib.optionalString stdenvNoCC.isLinux '' | |
install -D -m 755 $src/material_maker.x86_64 $out/opt/material-maker | |
install -D -m 644 $src/material_maker.pck $out/opt/material-maker.pck | |
for dir in library nodes environments meshes export; do | |
cp -R $src/$dir $out/opt | |
done | |
cp -R $src/examples $examples | |
mkdir -p $out/share/ | |
cp -R $src/doc $out/share | |
install -D -m 644 $src/doc/_static/icon.png -t $out/share/icons/hicolor/256x256/apps/ | |
mkdir -p $out/bin | |
ln -s $out/opt/material-maker $out/bin/material-maker | |
'' | |
+ lib.optionalString (builtins.elem "doc" finalAttrs.meta.outputsToInstall && stdenvNoCC.isLinux) '' | |
ln -s $doc/share/doc $out/opt/doc | |
'' | |
+ | |
lib.optionalString (builtins.elem "examples" finalAttrs.meta.outputsToInstall && stdenvNoCC.isLinux) | |
'' | |
ln -s $examples $out/opt/examples | |
'' | |
+ lib.optionalString stdenvNoCC.isDarwin '' | |
mkdir -p $out/Applications | |
find ./ -type d -name "*.app" -exec cp -r {} /destination/directory $out/Applications \; | |
'' | |
+ '' | |
runHook postInstall | |
''; | |
installPhase = | |
'' | |
runHook preInstall | |
'' | |
+ lib.optionalString stdenvNoCC.isLinux '' | |
install -D -m 755 $src/material_maker.x86_64 $out/opt/material-maker | |
install -D -m 644 $src/material_maker.pck $out/opt/material-maker.pck | |
for dir in library nodes environments meshes export; do | |
cp -R $src/$dir $out/opt | |
done | |
install -D -m 644 $src/doc/_static/icon.png -t $out/share/icons/hicolor/256x256/apps/ | |
mkdir -p $out/bin | |
ln -s $out/opt/material-maker $out/bin/material-maker | |
cp -R $src/doc $out/share | |
cp -R $src/examples $examples | |
for output in $(getAllOutputNames); do | |
if [ "$output" == "doc" ]; then | |
ln -s $doc/share/doc $out/opt/doc | |
elif [ "$output" == "examples" ]; then | |
ln -s $examples $out/opt/examples | |
fi | |
done | |
'' | |
+ lib.optionalString stdenvNoCC.isDarwin '' | |
mkdir -p $out/Applications | |
find ./ -type d -name "*.app" -exec cp -r {} /destination/directory $out/Applications \; | |
'' | |
+ '' | |
runHook postInstall | |
''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it but changed it so it queries outputsToInstall instead, i think this is wat it needs to follow, correct me if im wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because if the user wants to only install the out and not doc and examples they will override the meta.outputsToInstall because if you remove outputs it doesnt build anymore
27bfd22
to
a1b6cfb
Compare
cp -r $src/doc $out/share | ||
cp -r $src/examples $examples | ||
|
||
for output in ${builtins.concatStringsSep " " finalAttrs.meta.outputsToInstall}; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for output in ${builtins.concatStringsSep " " finalAttrs.meta.outputsToInstall}; do | |
for output in $(getAllOutputNames); do |
As I understand it, meta.outputsToInstall
is used for the default outputs, which isn't a problem here since doc
and examples
are in the default already.
However, we will typically change outputs
, so we need to match all cases from that. Also, if meta.outputsToInstall
changes, the package will be rebuilt, and we don't want that to happen as changes to meta shouldn't cause a rebuild.
Example, try to build with:
nix-build -E 'with import ./. {}; (material-maker.overrideAttrs { outputs = ["out" "doc"]; })'
-
If
outputsToInstall = finalAttrs.outputs;
: No problem since overridingoutputs
will also overrideoutputsToInstall
and everything works. However, this doesn't sound correct to me becauseoutputsToInstall
is the default and shouldn't depend onoutputs
. -
If we pin
outputsToInstall = [ "out" "doc" "examples" ];
: This doesn't work since we're looping overoutputsToInstall
which has "examples", but the overriden outputs does not, and so the destination$examples
will not exist.
platforms = lib.platforms.darwin ++ (lib.intersectLists lib.platforms.linux lib.platforms.x86_64); | ||
maintainers = with lib.maintainers; [ aurreland ]; | ||
mainProgram = "material-maker"; | ||
outputsToInstall = finalAttrs.outputs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to specify the outputs directly, here:
outputsToInstall = finalAttrs.outputs; | |
outputsToInstall = [ | |
"out" | |
"doc" | |
"examples" | |
]; |
If outputs
change, that shouldn't change the default outputs, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we should move
cp -r $src/doc $out/share
cp -r $src/examples $examples
into the for loop ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was kinda thinking the same as well, sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @aurreland and @d-brasher for your amazing contributions.
This looks good to me 😁
Result of 3 packages built:
Also tested basic app functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, too!
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/1881 |
I think we can remove finalAttrs, I'll do it this evening if you don't see any problem. |
Its currently not being used in the package. And I don't know if it's useful to have it in. It's difficult to find good docs about it. |
And if we keep it, should we use less let in and more finalAttrs refs? |
This issue is the only thing I can find about You could indeed drop the use of If you choose to keep the |
I'll look into moving everything into finalAttrs. |
Does this looks good to you ? I'll squash when its finalized |
looks fine to me 👍 |
Co-Authored-By: d-brasher <175485311+d-brasher@users.noreply.github.com> Co-Authored-By: Fedi Jamoussi <Fedi.Jamoussi@protonmail.ch>
We should be done this time |
Description of changes
Material Maker is a Tool based on Godot Engine that can be used to create textures procedurally and paint 3D models.
https://www.materialmaker.org/
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.