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

mandelbulber: init at 2.20 #66390

Merged
merged 1 commit into from Dec 6, 2019
Merged

Conversation

@KoviRobi
Copy link
Contributor

@KoviRobi KoviRobi commented Aug 9, 2019

Tested on my machine (NixOS 19.03.173243.9ef3cb9b0ba, x86_64), see package request #52038

Motivation for this change

I wanted to generate pretty pictures.

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
  • 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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Copy link
Contributor

@aanderse aanderse left a comment

Looking good. After a quick review I only have cosmetic points to mention. 👍

pkgs/applications/graphics/mandelbulber2/default.nix Outdated Show resolved Hide resolved
pkgs/applications/graphics/mandelbulber2/default.nix Outdated Show resolved Hide resolved
pkgs/applications/graphics/mandelbulber2/default.nix Outdated Show resolved Hide resolved
pkgs/applications/graphics/mandelbulber2/default.nix Outdated Show resolved Hide resolved
pkgs/applications/graphics/mandelbulber2/default.nix Outdated Show resolved Hide resolved
pkgs/applications/graphics/mandelbulber2/default.nix Outdated Show resolved Hide resolved
@KoviRobi KoviRobi force-pushed the KoviRobi:mandelbulber2-init-at-2.19 branch from 3044ce8 to d7ae974 Sep 1, 2019
@ivan
Copy link
Member

@ivan ivan commented Sep 1, 2019

@GrahamcOfBorg build mandelbulber2

@ivan
ivan approved these changes Sep 1, 2019
Copy link
Member

@ivan ivan left a comment

Built and tested on NixOS x86_64, examples load and both CPU and OpenCL rendering work. LGTM.

@KoviRobi KoviRobi force-pushed the KoviRobi:mandelbulber2-init-at-2.19 branch from d7ae974 to ca4f23d Sep 2, 2019
@KoviRobi
Copy link
Contributor Author

@KoviRobi KoviRobi commented Sep 2, 2019

While it should work on Mac and Windows as well, since I don't have either to test it I haven't added it to the platforms, my thinking was that if someone needs it on that platform they can test it and make it work.

@KoviRobi KoviRobi force-pushed the KoviRobi:mandelbulber2-init-at-2.19 branch 2 times, most recently from 126f490 to 8090518 Sep 2, 2019
@KoviRobi
Copy link
Contributor Author

@KoviRobi KoviRobi commented Sep 2, 2019

Apparently there are two flags to pass to the C compiler/preprocessor, into which I want a C string, hence I need to enclose the path in double-quotes. Make calls the C compiler in a shell, so I need to escape the double-quotes hence ("). The shell calling qmake does backslash expansion hence (\"); Nix strings do backslash expansion hence (\\\").

@ivan
ivan approved these changes Sep 2, 2019
Copy link
Member

@ivan ivan left a comment

Built and tested again, LGTM

@aanderse
Copy link
Contributor

@aanderse aanderse commented Oct 12, 2019

ping (triage) ... where are we at with this?

@KoviRobi
Copy link
Contributor Author

@KoviRobi KoviRobi commented Oct 15, 2019

I have folded in the latest changes from jtojnar, and have asked upstream to patch example files buddhi1980/mandelbulber2#719 though that hasn't progressed yet. Apart from that I think that's all the comments addressed

@aanderse
Copy link
Contributor

@aanderse aanderse commented Oct 17, 2019

@KoviRobi Please address bot failures when you get a chance.

@KoviRobi
Copy link
Contributor Author

@KoviRobi KoviRobi commented Oct 18, 2019

Sure, though not sure how to interpret the failures, all I see is

grahamcofborg-eval

x86_64-linux	mandelbulber2
i686-linux	mandelbulber2
aarch64-linux	mandelbulber2

grahamcofborg-eval-check-meta

nix-env failed:

and it doesn't say much to me.

@KoviRobi
Copy link
Contributor Author

@KoviRobi KoviRobi commented Oct 26, 2019

Looks like the bot just needed to be started again?

@KoviRobi
Copy link
Contributor Author

@KoviRobi KoviRobi commented Nov 19, 2019

@aanderse I think this is ready to be merged? I have addressed all the comments, and the bot has managed to make everything pass.

@aanderse
Copy link
Contributor

@aanderse aanderse commented Nov 19, 2019

@GrahamcOfBorg build mandelbulber2

@aanderse
Copy link
Contributor

@aanderse aanderse commented Nov 19, 2019

@KoviRobi sorry for the delay, this PR slipped by me. Thanks for your patience and persistence!

@ivan @jtojnar any additional changes required on your part before merge?

@aanderse
Copy link
Contributor

@aanderse aanderse commented Nov 22, 2019

@KoviRobi any idea on whenthe next upstream release is? That release will eliminate the need for your patch here as your patch was accepted upstream...

@KoviRobi
Copy link
Contributor Author

@KoviRobi KoviRobi commented Nov 22, 2019

@aanderse I have just asked, apparently in about 2 weeks, so might be worth the wait until then

@KoviRobi KoviRobi force-pushed the KoviRobi:mandelbulber2-init-at-2.19 branch from 10e390c to 7ba91ac Dec 5, 2019
@KoviRobi KoviRobi changed the title mandelbulber2: init at 2.19 mandelbulber2: init at 2.20 Dec 5, 2019
@KoviRobi
Copy link
Contributor Author

@KoviRobi KoviRobi commented Dec 5, 2019

Now using the new version, 2.20 which has the hardcoded paths patch included

Copy link
Contributor

@jtojnar jtojnar left a comment

Just a few finishing suggestions, mostly style suggestions for cleaner diffs, almost ready to merge.

pkgs/applications/graphics/mandelbulber2/default.nix Outdated Show resolved Hide resolved
pkgs/applications/graphics/mandelbulber2/default.nix Outdated Show resolved Hide resolved
pkgs/applications/graphics/mandelbulber2/default.nix Outdated Show resolved Hide resolved
pkgs/applications/graphics/mandelbulber2/default.nix Outdated Show resolved Hide resolved
pkgs/applications/graphics/mandelbulber2/default.nix Outdated Show resolved Hide resolved
@KoviRobi KoviRobi force-pushed the KoviRobi:mandelbulber2-init-at-2.19 branch from bb23672 to 5ce0f6a Dec 6, 2019
@KoviRobi KoviRobi requested a review from ttuegel as a code owner Dec 6, 2019
@KoviRobi KoviRobi force-pushed the KoviRobi:mandelbulber2-init-at-2.19 branch from 5ce0f6a to bb23672 Dec 6, 2019
@KoviRobi
Copy link
Contributor Author

@KoviRobi KoviRobi commented Dec 6, 2019

Whoops, I seem to have messed up something with the commits, I just wanted to squash it into a single commit (I think that's preferred in the nixpkgs manual), let me fix that

@KoviRobi KoviRobi force-pushed the KoviRobi:mandelbulber2-init-at-2.19 branch from bb23672 to 2cab8bf Dec 6, 2019
@KoviRobi
Copy link
Contributor Author

@KoviRobi KoviRobi commented Dec 6, 2019

Sorry about the accidental mistake, I just did a git rebase -i HEAD~5 thinking that just leaving the extra commits as above won't change anything, but it did (probably should have used git rebase -i HEAD^5, I just didn't pay close enough attention to the difference between ~ and ^, now I know). This also then triggered a review request that I now don't know how to cancel, sorry ttuegel.

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Dec 6, 2019

On the website and everywhere else, it seems to be referred as just mandelbulber. Would it make sense drop 2 from pname and attribute name? Also that is what it goes by on repology: https://repology.org/projects/?search=mandelbulber&maintainer=&category=&inrepo=&notinrepo=&repos=&families=&repos_newest=&families_newest=

@KoviRobi
Copy link
Contributor Author

@KoviRobi KoviRobi commented Dec 6, 2019

I guess so, I just got the impression that it went by mandelbulber2 for the version 2 (https://repology.org/project/mandelbulber/versions), though it's not a fork and v1 is deprecated (https://sourceforge.net/projects/mandelbulber/files/). Don't know if there was a rewrite/redesign that made or other incompatibilities that made packaging the two separately worthwhile. But anyway we don't have a v1.

Tested on my machine, see package request #52038
@KoviRobi KoviRobi force-pushed the KoviRobi:mandelbulber2-init-at-2.19 branch from 2cab8bf to d6c37ff Dec 6, 2019
@KoviRobi KoviRobi changed the title mandelbulber2: init at 2.20 mandelbulber: init at 2.20 Dec 6, 2019
@jtojnar jtojnar merged commit 866dd4c into NixOS:master Dec 6, 2019
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
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
mandelbulber on aarch64-linux Success
Details
mandelbulber on x86_64-linux Success
Details
@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Dec 6, 2019

Great work, thanks.

dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Dec 6, 2019
mandelbulber: init at 2.20
(cherry picked from commit 866dd4c)
@KoviRobi KoviRobi deleted the KoviRobi:mandelbulber2-init-at-2.19 branch Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.