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

soil: fix dead url, enable on darwin #101042

Merged
merged 1 commit into from Nov 7, 2020
Merged

soil: fix dead url, enable on darwin #101042

merged 1 commit into from Nov 7, 2020

Conversation

@r-burns
Copy link
Contributor

@r-burns r-burns commented Oct 18, 2020

Added myself as a maintainer as this appears to be orphaned.

The new github source appears to be the most popular mirror, and has a number of bugfixes along with a more sophisticated cmake build system which works on macos with no modifications.

The only dependee is already marked as broken, but I've successfully used this in another package I'm hacking on.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.
@r-burns
Copy link
Contributor Author

@r-burns r-burns commented Oct 23, 2020

/marvin opt-in
/status needs_reviewer

@marvin-mk2 marvin-mk2 bot added the marvin label Oct 23, 2020
@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Oct 23, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@aanderse
Copy link
Member

@aanderse aanderse commented Oct 27, 2020

@7c6f434c a bit of a wild guess, but I have the feeling you might be reasonably equipped to review this PR... From a quick overview LGTM 👍

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Oct 27, 2020

I have no idea… so this is a cleanup by a completely unrelated (and also not well-known) developer. I haven't looked at the code because I have never used that thing anyway…

So what do we do in such cases? Compare the initial commit with the original source from binary cache or Web Archive, then review the diffs? Dunno.

@r-burns
Copy link
Contributor Author

@r-burns r-burns commented Oct 27, 2020

Hmm yeah that's fair, I just thought the cmake cleanup was convenient because it would do most of the cross-platform compat work for us. Would it be preferable to grab it from another repo's archive? I see a handful in https://repology.org/project/soil/versions.

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Oct 28, 2020

For the record, the changes seem reasonably limited and useful (I haven't had energy to look into detailed diffs yet, though) — so I do not think we should take on an unsurmountabel verification burden, but it is a good idea to discuss how much we can verify in reasonable time and how much of that we want to do before making a call.

@r-burns
Copy link
Contributor Author

@r-burns r-burns commented Oct 29, 2020

Yeah definitely, I wouldn't want to introduce this burden for such little gain. I think the issue is just that the existing makefile isn't written in a very generic way. Alternatively, what about just hard-coding the compile commands? Looks damn ugly, but is quite generic, and doesn't introduce any third-party code.

diff --git a/pkgs/development/libraries/soil/default.nix b/pkgs/development/libraries/soil/default.nix
index cf0896170c1..acf047cd6ef 100644
--- a/pkgs/development/libraries/soil/default.nix
+++ b/pkgs/development/libraries/soil/default.nix
@@ -10,10 +10,17 @@ stdenv.mkDerivation {
 
   buildInputs = [ unzip mesa libGL libX11 ];
 
-  sourceRoot = "Simple OpenGL Image Library/projects/makefile";
-  preBuild   = "mkdir obj";
-  preInstall = "mkdir -p $out/lib $out/include";
-  makeFlags  = [ "LOCAL=$(out)" ];
+  buildPhase = ''
+    cd src
+    $CC $NIX_CFLAGS_COMPILE -c *.c
+    $AR r libSOIL.a *.o
+    $RANLIB libSOIL.a
+  '';
+  installPhase = ''
+    mkdir -p $out/lib $out/include/SOIL
+    cp libSOIL.a $out/lib/
+    cp SOIL.h $out/include/SOIL/
+  '';
 
   meta = {
     description     = "Simple OpenGL Image Library";

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Oct 29, 2020

Looks fine to me, actually, when everything is flat and we know the libraries do not need to be searched for, compiling by hand is not that much of a burden.

I guess finding some now-available source copies for non-binary-cache users would still be a good idea, but maybe archive.org has cached the exact same file?

@r-burns
Copy link
Contributor Author

@r-burns r-burns commented Oct 29, 2020

Ah, finally got the archive download to work and produce the same hash. Will push shortly.

@r-burns r-burns changed the title soil: use github source, enable on darwin soil: fix dead url, enable on darwin Nov 3, 2020
@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Nov 3, 2020

I think it needs more, like libX11

@r-burns
Copy link
Contributor Author

@r-burns r-burns commented Nov 4, 2020

Doesn't seem to need it for me. Or do you mean it should propagate a libX11 dependency to its consumers?

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Nov 6, 2020

@ofborg build soil

@r-burns r-burns force-pushed the soil branch 2 times, most recently from ebf7c5d to 62407ab Nov 6, 2020
@r-burns
Copy link
Contributor Author

@r-burns r-burns commented Nov 6, 2020

Sorry, could have sworn I had tested that on linux :/

@7c6f434c 7c6f434c merged commit d464bdc into NixOS:master Nov 7, 2020
19 checks passed
@r-burns r-burns deleted the soil branch Nov 11, 2020
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

3 participants