Fix: GlusterFS python tools all have import errors #25709

nh2 · 2017-05-11T16:39:42Z

Motivation for this change

Fixes #25620 - GlusterFS python tools all have import errors

Also enables parallel building, after I have checked it (see last commit).

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

mention-bot · 2017-05-11T16:39:44Z

@nh2, thanks for your PR! By analyzing the history of the files in this pull request, we identified @7c6f434c, @fduraffourg and @proger to be potential reviewers.

nh2 · 2017-05-11T16:40:36Z

CC @cleverca22 @FRidh

7c6f434c · 2017-05-11T17:21:06Z

Re: checking on upgrade: maybe run --help on everything in $out/bin so that a new problem will not go unnoticed?

nh2 · 2017-05-11T17:30:36Z

Re: checking on upgrade: maybe run --help on everything in $out/bin so that a new problem will not go unnoticed?

@7c6f434c I can't do that, because not all programs accept --help (see for example gluster being commented out).

FRidh

Looks excellent. I like to have that link in but aside from that its good for me.

FRidh · 2017-05-11T17:22:50Z

pkgs/tools/filesystems/glusterfs/default.nix

+  # By default they are indeterministic because such files contain time stamps
+  # (see https://nedbatchelder.com/blog/200804/the_structure_of_pyc_files.html).
+  # So we use the same environment variables as in
+  #   https://github.com/NixOS/nixpkgs/blob/249b34aadca7038207492f29142a3456d0cecec3/pkgs/development/interpreters/python/mk-python-derivation.nix#L61


please refer instead or also to #25707

FRidh · 2017-05-11T17:26:08Z

pkgs/tools/filesystems/glusterfs/default.nix

+    # NOTE: `python2` has to be *AFTER* the above `python2.withPackages`,
+    #       to ensure that the packages are available but the `toPythonPath`
+    #       shell function used in `postFixup` is also still available.
+    python2


Interesting solution. Perhaps we should move addPythonPath and toPythonPath into a derivation although I hope we can get rid of it in the future.

@cleverca22 came up with it (thanks a lot for that).

We could also just inherit the hooks in python.buildEnv

FRidh · 2017-05-11T17:27:32Z

pkgs/tools/filesystems/glusterfs/default.nix

+    # for example for `peer_georep-sshkey` key generation, so `$out/bin` is needed in the PATH.
+    # It also invokes executable Python scripts in `$out/libexec/glusterfs`, which is why we set up PYTHONPATH accordingly.
+    # Because the user may invoke `gluster` directly, we also set up the paths for that executable.
+    wrapProgram $out/bin/glusterd --set PATH "${stdenv.lib.makeBinPath [ openssh ]}:$out/bin" --set PYTHONPATH "$(toPythonPath $out)"


I think you're the first to combine withPackages with PYTHONPATH. Looking at what we available I think this is the best solution.

instead of everywhere putting $(toPythonPath $out):$out/libexec/glusterfs you may instead do PYTHONPATH=$(toPythonPath $out):$out/libexec/glusterfs and reuse that. I think it becomes more readable that way.

FRidh · 2017-05-11T17:31:07Z

pkgs/tools/filesystems/glusterfs/default.nix

+
+    wrapProgram $out/share/glusterfs/scripts/eventsdash.py --set PYTHONPATH "$(toPythonPath $out)" --set PATH "$out/bin"
+
+    # Tests that the above programs work without import errors.


This is for the installCheckPhase. Don't forget to enable it.

@FRidh Is the installCheckPhase still allowed to generate .pyc files?

I think so. As far as I know the store is RW during the whole build.

nh2 · 2017-05-11T17:32:14Z

I may be wrong but the travis build failure seems to be unrelated to my PR.

FRidh · 2017-05-11T17:33:14Z

@nh2 you can typically ignore Travis

7c6f434c · 2017-05-11T18:34:36Z

Re: --help: well, a paranoid approach would be to use blacklist instead of whitelist (builder cannot do much damage anyway).

Also, I wonder if wrapping everything with python in the #! line would be a good idea.

nh2 · 2017-05-12T02:20:00Z

I found that some gluster utils need even more deps, don't merge yet.

nh2 · 2017-05-14T23:28:59Z

I have revised this PR, adding new commits and patches to the glusterfs source code.

With these, the GlusterFS geo-replication feature now works correctly without any Python or other errors.

Please take another look.

FRidh · 2017-05-15T06:44:31Z

You could use substituteInPlace to substitute library name with paths to the libraries. That's how we typically deal with ctypes/cffi in python-packages.nix.

In any case, this looks good to me.

7c6f434c · 2017-05-15T08:03:55Z

pkgs/tools/filesystems/glusterfs/default.nix

+    wrapProgram $out/sbin/mount.glusterfs --set PATH "$GLUSTER_PATH" --set PYTHONPATH "$GLUSTER_PYTHONPATH" --set LD_LIBRARY_PATH "$GLUSTER_LD_LIBRARY_PATH"
+
+    # Set Python environment for the Python based utilities.
+    # It would be nice if there was a better way to do this, automatically for all of them.


Would checking for #! …/python be a good way to perform a sanity check that nothing obvious is missed?

nh2 · 2017-05-19T14:24:47Z

I've updated the PR with the fix for #25620 (comment), and also added the upgrade to glusterfs 3.10.2 which was released in the meantime.

FRidh · 2017-05-19T14:32:19Z

nixos/modules/services/network-filesystems/glusterfs.nix

@@ -52,6 +52,8 @@ in

      preStart = ''
        install -m 0755 -d /var/log/glusterfs
+        mkdir -p /var/lib/glusterd/hooks/
+        ${rsync}/bin/rsync -a ${glusterfs}/var/lib/glusterd/hooks/ /var/lib/glusterd/hooks/


Why copy them and not link to them in the store?

@FRidh I do not know what gluster's assumptions are about these files being writable.

I believe the right way to do this is to have gluster not expect these files that are required for operation in /var, but inside its install prefix instead. People on the #gluster IRC channel seem to share this opinion.

I have filed an upstream bug about moving those files: https://bugzilla.redhat.com/show_bug.cgi?id=1452761

I've also added a comment about this right above preStart.

They were made unnecessary in commit d07154b, which added `makeFlags = "DESTDIR=$(out)";`.

This is because the source tarball available on https://download.gluster.org/pub/gluster/glusterfs/3.10/3.10.1/glusterfs-3.10.1.tar.gz has different contents than the v3.10.1 tag; for example, it lacks the file `xlators/features/ganesha/src/Makefile.am`, which the tag has. This is because GluserFS's release process removes some unused files. This made impossible to apply patches written by or for upstream, as those are written against what's in upstream's git. As a nice side effect, we no longer have to hardcode the "3.10" in the `3.10/${version}` part of the URL.

Done by setting PATH and PYTHONPATH appropriately. Adds the following patches: * One that removes hardcodes to /sbin, /usr/bin, etc. from gluster, so that programs like `lvm` and `xfs_info` can be called at runtime; see https://bugzilla.redhat.com/show_bug.cgi?id=1450546. * One that fixes unsubstituted autoconf macros in paths (a problem in the 3.10 release); see https://bugzilla.redhat.com/show_bug.cgi?id=1450588. * One that removes uses of the `find_library()` Python function that does not behave as expected in Python < 3.6 (and would not behave correctly even on 3.6 in nixpkgs due to NixOS#25763); see https://bugzilla.redhat.com/show_bug.cgi?id=1450593. I think that all of these patches should be upstreamed. Also adds tests to check that none of the Python based utilities throw import errors, calling `--help` or equivalent on them.

I checked for determinism with `nix-build --option build-repeat 10`.

This is where glusterfs expects them; see also https://github.com/gluster/glusterfs/blob/v3.10.1/extras/hook-scripts/Makefile.am#L4 Also see upstream bug https://bugzilla.redhat.com/show_bug.cgi?id=1452761

FRidh · 2017-05-30T07:47:21Z

I forgot about this PR. @nh2 this was ready to go in, right?

nh2 · 2017-06-01T20:03:24Z

@FRidh Yes

nh2 mentioned this pull request May 11, 2017

GlusterFS python tools all have import errors #25620

Closed

FRidh requested changes May 11, 2017

View reviewed changes

nh2 force-pushed the 25620-glusterfs-fix-python-import-errors branch from 0f08606 to f65bada Compare May 11, 2017 18:02

nh2 force-pushed the 25620-glusterfs-fix-python-import-errors branch 2 times, most recently from 9899b19 to 6c8abd5 Compare May 14, 2017 17:34

FRidh approved these changes May 15, 2017

View reviewed changes

7c6f434c reviewed May 15, 2017

View reviewed changes

7c6f434c approved these changes May 15, 2017

View reviewed changes

nh2 force-pushed the 25620-glusterfs-fix-python-import-errors branch from 6c8abd5 to f8be568 Compare May 19, 2017 14:21

FRidh reviewed May 19, 2017

View reviewed changes

nh2 added 6 commits May 19, 2017 16:56

glusterfs: Remove no longer needed subsitutions of DESTDIR.

2f7152f

They were made unnecessary in commit d07154b, which added `makeFlags = "DESTDIR=$(out)";`.

glusterfs: Enable parallel building.

01bbdae

I checked for determinism with `nix-build --option build-repeat 10`.

glusterfs service: Copy hooks to /var at startup.

ff4eb1e

This is where glusterfs expects them; see also https://github.com/gluster/glusterfs/blob/v3.10.1/extras/hook-scripts/Makefile.am#L4 Also see upstream bug https://bugzilla.redhat.com/show_bug.cgi?id=1452761

glusterfs: Upgrade to 3.10.2

13eefe1

nh2 force-pushed the 25620-glusterfs-fix-python-import-errors branch from f8be568 to 13eefe1 Compare May 19, 2017 14:56

FRidh merged commit 87ee589 into NixOS:master Jun 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: GlusterFS python tools all have import errors #25709

Fix: GlusterFS python tools all have import errors #25709

nh2 commented May 11, 2017

mention-bot commented May 11, 2017

nh2 commented May 11, 2017

7c6f434c commented May 11, 2017

nh2 commented May 11, 2017

FRidh left a comment

FRidh May 11, 2017

nh2 May 11, 2017

FRidh May 11, 2017

nh2 May 11, 2017

FRidh May 11, 2017

FRidh May 11, 2017

FRidh May 11, 2017

nh2 May 11, 2017

FRidh May 11, 2017

nh2 May 11, 2017 •

edited

FRidh May 11, 2017

nh2 commented May 11, 2017

FRidh commented May 11, 2017

7c6f434c commented May 11, 2017

nh2 commented May 12, 2017

nh2 commented May 14, 2017

FRidh commented May 15, 2017

7c6f434c May 15, 2017

nh2 commented May 19, 2017

FRidh May 19, 2017

nh2 May 19, 2017

nh2 May 19, 2017

FRidh commented May 30, 2017

nh2 commented Jun 1, 2017


		wrapProgram $out/share/glusterfs/scripts/eventsdash.py --set PYTHONPATH "$(toPythonPath $out)" --set PATH "$out/bin"

		# Tests that the above programs work without import errors.

Fix: GlusterFS python tools all have import errors #25709

Fix: GlusterFS python tools all have import errors #25709

Conversation

nh2 commented May 11, 2017

Motivation for this change

Things done

mention-bot commented May 11, 2017

nh2 commented May 11, 2017

7c6f434c commented May 11, 2017

nh2 commented May 11, 2017

FRidh 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

nh2 May 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nh2 commented May 11, 2017

FRidh commented May 11, 2017

7c6f434c commented May 11, 2017

nh2 commented May 12, 2017

nh2 commented May 14, 2017

FRidh commented May 15, 2017

Choose a reason for hiding this comment

nh2 commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FRidh commented May 30, 2017

nh2 commented Jun 1, 2017

nh2 May 11, 2017 •

edited