obj2voxel: init at 1.3.4 #254299

zeuner · 2023-09-09T21:47:08Z

Description of changes

Add package for obj2voxel. Fixes #254298.

Things done

dotlambda

meta.maintainers is missing and the commit message should follow https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions

pkgs/tools/graphics/obj2voxel/default.nix

nagy · 2023-09-10T09:10:37Z

maintainers/maintainer-list.nix

The contents of this file should be commited in a separate commit. You can see the history of that file for good examples of commit messages. Also it needs the githubid. You can look at the top of the file for instructions on how to obtain it.

https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions

Will split the commit. AFAIK, the current master branch doesn't require a githubId, though: https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix#L7

Oh, good to know. Then the split should be enough.

Please add the githubId. It helps if you ever change your GitHub handle.
Definitely add github. Otherwise you can't be pinged by https://github.com/ofborg when someone makes a change to this package.

The more likely scenario would be that I delete my Github handle once I figure out how to deal with various vendor lock-in issues that prevent this ATM.

Feel free to remove your GitHub handle from this file once you do so.

nagy · 2023-09-10T09:11:49Z

pkgs/tools/graphics/obj2voxel/default.nix

+
+  patches = [
+    # pull missing definitions (https://github.com/Eisenwave/voxel-io/pull/3)
+    ./stdlib-defs.patch


Could you use fetchpatch with the url https://github.com/Eisenwave/voxel-io/pull/3.patch instead of commiting the file ?

I have to look into that. Actually, I first thought about pulling the patch from the commit in the PR mentioned, but it's for a submodule, so it would have to be applied to a subdirectory of the source tree.

Its not a blocker. But when patches are available in a more or less stable url, then putting them in the tree is not really needed.

@nagy If I figure out a clean way to apply an external patch to a subdirectory of the source tree, I would actually prefer getting it from the PR. This would avoid duplicated effort since the build errors on aarch64-darwin I see through ofborg should also be pushed upstream.

@dotlambda The more likely scenario would be that I delete my Github handle once I figure out how to deal with various vendor lock-in issues that prevent this ATM.

fetchpatch can add a prefix

I moved most patches to corresponding upstream PRs (Eisenwave/voxel-io#4, Eisenwave/voxel-io#3) and switched to fetchpatch. The remaining patch is specific to 1.3.4 as the upstream head already fixed it.

nagy · 2023-09-10T09:13:51Z

pkgs/tools/graphics/obj2voxel/default.nix

+      GIT_CONFIG_KEY_0 = "url.https://github.com/.insteadOf";
+      GIT_CONFIG_VALUE_0 = "git@github.com:";
+    }
+  );


Interesting construction. I assume that your firewall has prevented from loading the submodules via ssh? I Dont know if this is the right way to circumvent this though.

It's a workaround suggested here: #195117 (comment)

Not sure whether it's better to require SSH keys on build hosts.

Ok, then lets keep it. Maybe it will be adapted in fetchgit itself one day.

nagy · 2023-09-10T09:15:52Z

pkgs/top-level/all-packages.nix

@@ -19882,6 +19882,8 @@ with pkgs;

  obelisk = callPackage ../development/tools/ocaml/obelisk { menhir = ocamlPackages.menhir; };

+  obj2voxel = callPackage ../tools/graphics/obj2voxel { };


Could you also switch this to the new by-name structure introduced in #237439 ?

Done, seems to work.

dotlambda

What about meta.homepage and meta.mainProgram?

pkgs/tools/graphics/obj2voxel/default.nix

zeuner · 2023-09-10T18:29:12Z

meta.maintainers is missing and the commit message should follow https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions

Should be resolved.

zeuner · 2023-09-10T18:31:05Z

What about meta.homepage and meta.mainProgram?

Not aware of a homepage, but mainProgram was added.

dotlambda · 2023-09-12T05:57:05Z

Not aware of a homepage

https://github.com/Eisenwave/voxel-io

dotlambda · 2023-09-12T05:56:07Z

pkgs/by-name/ob/obj2voxel/package.nix

+    (
+      fetchpatch {
+        name = "fix-aarch64-darwin-type-mismatch.patch";
+        url = "https://patch-diff.githubusercontent.com/raw/Eisenwave/voxel-io/pull/4.diff";


Use a commit url like https://github.com/Eisenwave/voxel-io/commit/2f800f6aca8acd0cc18f9625b411c6dc2956f2ea.patch because a push to the PR branch would invalidate the hash.

Using a pull-related patch URL was suggested earlier, and I actually think it makes sense. In case the PR would change, it is likely that the changes are also useful for the maintenance of the nix package, thus justifying an update of the hash.

We want to be able to build this package in the far future too so we need stable URLs.

Being able to build packages in the far future is a formidable goal. But if it is to be possible without resorting to cache.nixos.org, I doubt this is the right place to achieve it, though. It's not how Nix package are usually designed. E.g. even with git sources that could be potentially long-term stable, packages tend to reference (potentially unstable) tags instead of cryptographic hashes. I often encounter packages where the URL resource can't be retrieved anymore, or the hash has changed. Which shows that Nix has decided to take a compromise here. Stable URLs are being considered, but may be avoided if there is a reason to.

There is also a reason to reference pull requests instead of commit hashes because pull requests also refer to a specific goal with respect to the upstream source, and possibly the discussion around it. This helps maintainers because later on when changing the package, they can decide whether that specific goal is still relevant.

Nonetheless, the URLs have been changed now to URLs with the objective to remain stable over time. I hope this can relieve your concerns, while still keeping the possibility to look up the related pull request easily.

You can and should always reference the relevant PR and describe what it does in a comment right next to the patch URL, again best of both worlds.

Added that comment. The stability of commit diff URLs should still not be overestimated, I also saw them disappear in case of force-pushes sometimes.

dotlambda · 2023-09-12T14:47:05Z

pkgs/by-name/ob/obj2voxel/package.nix

@@ -67,6 +67,7 @@ stdenv.mkDerivation (finalAttrs: {
  meta = with lib; {
    description = "Convert OBJ and STL files to voxels, with support for textures";
    mainProgram = "obj2voxel";
+    homepage = "https://github.com/Eisenwave/obj2voxel/blob/master/README.adoc";


Suggested change

homepage = "https://github.com/Eisenwave/obj2voxel/blob/master/README.adoc";

homepage = "https://github.com/Eisenwave/obj2voxel";

Any rationale for this requested change? When looking for a homepage, people usually expect a page where they can read about the project, and not a list of files (which is what is presented when just opening the repo URL). So the README file is what comes closest to a homepage.

The README is right there when scrolling down, so the request actually allows both users and developers get what they want.

Changed it to point right to the repo as this is how many other packages also do it currently. I still think repo and homepage are not synonymous, and there ought to be an extra field, with the current meaning of "homepage" with that fallback being emulated by doing a meta.homepage or meta.repo.

I sometimes abuse meta.downloadPage for the repo url.

dotlambda · 2023-09-12T14:48:28Z

maintainers/maintainer-list.nix

+  gm6k = {
+    email = "nix@quidecco.pl";
+    name = "Isidor Zeuner";
+  };


github and githubId are still missing

Still not required: https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix#L7
"Optional, but at least one of email, matrix or githubId must be given"

Let's focus on issues relevant to this PR...

dotlambda · 2023-09-12T14:48:47Z

pkgs/by-name/ob/obj2voxel/package.nix

+, stdenv
+, fetchFromGitHub
+, fetchpatch
+, static ? stdenv.hostPlatform.isStatic


Suggested change

, static ? stdenv.hostPlatform.isStatic

dotlambda · 2023-09-12T14:50:01Z

pkgs/by-name/ob/obj2voxel/package.nix

+    (
+      fetchpatch {
+        name = "fix-aarch64-darwin-type-mismatch.patch";
+        url = "https://patch-diff.githubusercontent.com/raw/Eisenwave/voxel-io/pull/4.diff";


We want to be able to build this package in the far future too so we need stable URLs.

dotlambda · 2023-09-12T14:50:45Z

pkgs/by-name/ob/obj2voxel/package.nix

+  '';
+
+  cmakeFlags = [
+    "-DBUILD_SHARED_LIBS=${if static then "OFF" else "ON"}"


Suggested change

"-DBUILD_SHARED_LIBS=${if static then "OFF" else "ON"}"

"-DBUILD_SHARED_LIBS=${if stdenv.targetPlatform.isStatic then "OFF" else "ON"}"

dotlambda · 2023-09-12T14:51:20Z

pkgs/by-name/ob/obj2voxel/package.nix

+    mainProgram = "obj2voxel";
+    homepage = "https://github.com/Eisenwave/obj2voxel/blob/master/README.adoc";
+    license = licenses.mit;
+    platforms = platforms.all;


Maybe

Suggested change

platforms = platforms.all;

platforms = platforms.unix;

is more appropriate.

This seems remarkable, considering that the package does not use any libraries, CMake is known to be very portable, and pure standard C++ even more. So I would have expected that almost any platform can work (modulo maybe patching around some quirks). But since I have no non-unix platform to test here, I'll have to trust your assessment.

dotlambda · 2023-09-12T14:51:45Z

pkgs/by-name/ob/obj2voxel/package.nix

+      }
+    )
+    # already fixed in upstream git head
+    ./1.3.4-fix-type-mismatch.patch


We can't use fetchpatch?

IMHO, not easily. By now, upstream master avoids the issue because all these size_t typed variables have been changed to usize. But this is a rather global change not easily applied, so I decided towards a clear and small patch that just solves the issue remaining after applying the fixes that made it into pull requests.

IMHO, the next best solution would actually be to just build the upstream master. But for now, I decided to keep with the usual policy of building something that has a version number.

dotlambda · 2023-09-12T14:52:11Z

pkgs/by-name/ob/obj2voxel/package.nix

+, fetchFromGitHub
+, fetchpatch
+, static ? stdenv.hostPlatform.isStatic
+, config


Suggested change

, config

zeuner · 2023-10-10T12:38:58Z

meta.maintainers is missing and the commit message should follow https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions

Fixed one month ago.

nagy

If you squash all obj2voxel: related commits into one init commit ( so that we have one package and one maintainer commit ) then its fine for me.

zeuner · 2023-10-10T23:10:15Z

If you squash all obj2voxel: related commits into one init commit ( so that we have one package and one maintainer commit ) then its fine for me.

Done.

nixos-discourse · 2023-12-14T20:24:34Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-handle-stuck-reviews/36883/1

rhendric · 2023-12-15T05:53:58Z

pkgs/by-name/ob/obj2voxel/package.nix

+    (
+      fetchpatch {
+        name = "add-std-includes.patch";
+        url = "https://web.archive.org/web/20230914180442/https://patch-diff.githubusercontent.com/raw/Eisenwave/voxel-io/pull/3.diff";


Suggested change

url = "https://web.archive.org/web/20230914180442/https://patch-diff.githubusercontent.com/raw/Eisenwave/voxel-io/pull/3.diff";

url = "https://github.com/Eisenwave/voxel-io/compare/d902568de2d4afda254e533a5ee9e4ad5fe7d2be...2f800f6aca8acd0cc18f9625b411c6dc2956f2ea.patch";

And likewise for the other patch?

Kind of excessive going through archive.org for these just to get a permalink, isn't it?

Changed that as requested. Still, I'd like to point out that @dotlambda previously asked for stable URLs from which building in the far future is possible (see #254299 (comment)). The most straightforward way I know to achieve this is archive.org. github.com is known to lose commit information through garbage collection, e.g. in case of forced pushes.

rhendric

Haven't tested, but looks reasonable.

nixos-discourse · 2023-12-17T02:20:14Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3111

SuperSandro2000 · 2023-12-17T14:15:52Z

pkgs/by-name/ob/obj2voxel/package.nix

+  src = (
+    fetchFromGitHub {
+      owner = "Eisenwave";
+      repo = finalAttrs.pname;


Suggested change

repo = finalAttrs.pname;

repo = "obj2voxel";

If we append something to pname, that would no longer be true

Doesn't sound unreasonable, but wouldn't it be more appropriate to apply this change using a treewide commit supplemented by some contributor documentation to be read by all contributors?

I'm asking because using pname to construct repo URLs seems to be very common even among new packages, as verified by doing

grep -r 'repo = .*pname' pkgs/by-name

(pkgs/by-name being a fairly new mechanism)

SuperSandro2000 · 2023-12-17T14:16:20Z

pkgs/by-name/ob/obj2voxel/package.nix

+  ).overrideAttrs (
+    _: {
+      GIT_CONFIG_COUNT = 1;
+      GIT_CONFIG_KEY_0 = "url.https://github.com/.insteadOf";


Would be nice if we could upstream the url change

Good idea, done: Eisenwave/obj2voxel#9

For the time being, the workaround might still be necessary.

Mic92 · 2023-12-18T18:57:16Z

I would not merge any new packages with @zeuner as the only maintainer until this is sorted: #273146 (comment)

zeuner · 2023-12-20T06:30:33Z

I would not merge any new packages with @zeuner as the only maintainer until this is sorted: #273146 (comment)

I'm not sure what you would want to "sort" in the PR you mentioned. @infinisil already merged a PR which implements the requested code changes about a week ago (#273332), so technically, the PR is done.

If you are unhappy with the outcome of a non-related PR, you may express that in the particular PR, or open a new one if appropriate. But if you don't have anything to add to this PR, it doesn't belong here.

zeuner · 2023-12-20T06:33:52Z

@mweinelt I saw your review comments (#254299 (comment) #254299 (comment)) and already addressed them. So I don't understand why you would mark this PR as a draft. If you see anything that should be changed in your opinion, please state it clearly so it can be addressed.

dotlambda changed the title ~~obj2voxel: new package~~ obj2voxel: init at 1.3.4 Sep 9, 2023

dotlambda added the 12. first-time contribution This PR is the author's first one; please be gentle! label Sep 9, 2023

dotlambda requested changes Sep 9, 2023

View reviewed changes

pkgs/tools/graphics/obj2voxel/default.nix Outdated Show resolved Hide resolved

pkgs/tools/graphics/obj2voxel/default.nix Outdated Show resolved Hide resolved

ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Sep 9, 2023

zeuner force-pushed the obj2voxel branch 2 times, most recently from 35ffdb0 to 84768e9 Compare September 10, 2023 08:28

nagy suggested changes Sep 10, 2023

View reviewed changes

dotlambda reviewed Sep 10, 2023

View reviewed changes

pkgs/tools/graphics/obj2voxel/default.nix Outdated Show resolved Hide resolved

zeuner force-pushed the obj2voxel branch from 84768e9 to f24f468 Compare September 10, 2023 12:07

zeuner mentioned this pull request Sep 10, 2023

Possible vendor lock-in issue related to maintainer services NixOS/ofborg#651

Open

dotlambda reviewed Sep 12, 2023

View reviewed changes

zeuner requested a review from dotlambda October 10, 2023 12:39

nagy suggested changes Oct 10, 2023

View reviewed changes

zeuner force-pushed the obj2voxel branch from df03970 to ec1ef63 Compare October 10, 2023 23:08

nagy approved these changes Oct 11, 2023

View reviewed changes

zeuner mentioned this pull request Dec 10, 2023

maintainer-list: make github, githubId mandatory fields #273220

Closed

13 tasks

zeuner force-pushed the obj2voxel branch from e0622ea to 01a563f Compare December 13, 2023 15:57

rhendric reviewed Dec 15, 2023

View reviewed changes

zeuner force-pushed the obj2voxel branch 2 times, most recently from d6a9b8e to 1ff13d6 Compare December 15, 2023 12:58

rhendric approved these changes Dec 16, 2023

View reviewed changes

SuperSandro2000 reviewed Dec 17, 2023

View reviewed changes

mweinelt marked this pull request as draft December 18, 2023 19:03

zeuner marked this pull request as ready for review December 20, 2023 06:34

zeuner force-pushed the obj2voxel branch 2 times, most recently from 229899b to 53d85a2 Compare December 21, 2023 01:52

obj2voxel: init at 1.3.4

ad3ba37

zeuner force-pushed the obj2voxel branch from 53d85a2 to ad3ba37 Compare January 1, 2024 14:14

		@@ -19882,6 +19882,8 @@ with pkgs;

		obelisk = callPackage ../development/tools/ocaml/obelisk { menhir = ocamlPackages.menhir; };

		obj2voxel = callPackage ../tools/graphics/obj2voxel { };

	homepage = "https://github.com/Eisenwave/obj2voxel/blob/master/README.adoc";
	homepage = "https://github.com/Eisenwave/obj2voxel";

	"-DBUILD_SHARED_LIBS=${if static then "OFF" else "ON"}"
	"-DBUILD_SHARED_LIBS=${if stdenv.targetPlatform.isStatic then "OFF" else "ON"}"

	url = "https://web.archive.org/web/20230914180442/https://patch-diff.githubusercontent.com/raw/Eisenwave/voxel-io/pull/3.diff";
	url = "https://github.com/Eisenwave/voxel-io/compare/d902568de2d4afda254e533a5ee9e4ad5fe7d2be...2f800f6aca8acd0cc18f9625b411c6dc2956f2ea.patch";

obj2voxel: init at 1.3.4 #254299

Are you sure you want to change the base?

obj2voxel: init at 1.3.4 #254299

Conversation

zeuner commented Sep 9, 2023 • edited Loading

Description of changes

Things done

dotlambda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotlambda left a comment

Choose a reason for hiding this comment

zeuner commented Sep 10, 2023

zeuner commented Sep 10, 2023

dotlambda commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeuner Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeuner commented Oct 10, 2023

nagy left a comment

Choose a reason for hiding this comment

zeuner commented Oct 10, 2023

nixos-discourse commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhendric left a comment

Choose a reason for hiding this comment

nixos-discourse commented Dec 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 commented Dec 18, 2023

zeuner commented Dec 20, 2023

zeuner commented Dec 20, 2023

zeuner commented Sep 9, 2023 •

edited

Loading

zeuner Oct 5, 2023 •

edited

Loading