mblaze: fix mcom to use file utility. #94881

guillaumecherel · 2020-08-07T17:17:40Z

Motivation for this change

mcom needs the file utility when attaching document with mcom -attach bla.txt.

Things done

I wrapped mcom to add file to PATH.

guillaumecherel · 2020-08-08T16:01:50Z

This is my first PR to nixpkgs. I welcome any comment!

alyssais

Thanks for the PR!

The commit seems to be using an email address that isn’t registered to your GitHub account. Is that intentional?

pkgs/applications/networking/mailreaders/mblaze/default.nix

guillaumecherel · 2020-08-08T17:20:16Z

There it is. Thanks for the tips! Ideally, the "full wrapping" should be done for all the other executables in the package, but I don't have time to take care of that right now.

alyssais

Can you squash this into a single commit? And I think your email address might still not be right — see how GitHub doesn’t render commits with your avatar.

alyssais · 2020-08-08T21:10:05Z

pkgs/applications/networking/mailreaders/mblaze/default.nix

@@ -1,11 +1,13 @@
-{ stdenv, lib, fetchFromGitHub, fetchpatch, libiconv, ruby ? null }:
+{ stdenv, lib, fetchFromGitHub, fetchpatch, libiconv, ruby ? null, makeWrapper, coreutils, file, gawk, gnugrep, gnused }:


Maybe break this long line at 80 or something? And optional arguments (ruby) usually come last.

alyssais · 2020-08-08T21:10:32Z

pkgs/applications/networking/mailreaders/mblaze/default.nix

@@ -26,6 +28,9 @@ stdenv.mkDerivation rec {
    install -Dm644 -t $out/share/zsh/site-functions contrib/_mblaze
  '' + lib.optionalString (ruby != null) ''
    install -Dt $out/bin contrib/msuck contrib/mblow
+    wrapProgram $out/bin/mcom \
+      --prefix PATH : ${lib.makeBinPath [ gawk coreutils file gnugrep gnused ]} \


Maybe order them alphabetically? Not a big deal though.

guillaumecherel · 2020-08-09T12:01:08Z

Alright, I think my email is fixed now. I also did the formatting, and wrapped all the executables.

I also squashed my commits into a single one locally, but now my local branch has diverged from my github branch and I can't push anymore. Should I create a new branch an redo the pull request?

guillaumecherel · 2020-08-09T13:47:59Z

I think it's done, all commits squashed into one.

alyssais

Thanks for your patience! We’re getting there!

alyssais · 2020-08-11T00:13:18Z

pkgs/applications/networking/mailreaders/mblaze/default.nix

+        --prefix PATH : ${lib.makeBinPath [
+	  coreutils file gawk gnugrep gnused less nano
+	]} \
+        --set EDITOR nano


This will override the user’s editor choice, which is an impurity we should definitely allow. The only reason I suggested it for testing is that your normal EDITOR would probably not be a full path, and we were emptying PATH. Similarly, we probably shouldn’t include less in the wrapper, because a user might configure a different PAGER. Overall, it comes down to whether something is an implementation detail (like awk) or whether something is a property of the environment the program is running in that should be customisable (like the editor or pager). Does that make sense?

alyssais · 2020-08-11T00:14:28Z

pkgs/applications/networking/mailreaders/mblaze/default.nix

+
+    for x in `find $out/bin "(" -type f -executable ")" -or -type l`
+    do
+      wrapProgram $x \


I think we should probably only wrap the programs that are shell scripts. The others probably don’t need it, and Oulu feel weird to me to wrap programs that didn’t need it.

It would also be totally fine to decide that the others are out of scope for this PR, and to just concentrate on mcom for now, and fix the others later or not at all. Up to you — depends how quickly you want to get this over with!

alyssais · 2020-08-11T00:15:22Z

pkgs/applications/networking/mailreaders/mblaze/default.nix

@@ -1,11 +1,14 @@
-{ stdenv, lib, fetchFromGitHub, fetchpatch, libiconv, ruby ? null }:
+{ coreutils, fetchFromGitHub, fetchpatch, file, gawk, gnugrep, gnused, less,
+  lib, libiconv, makeWrapper, nano, stdenv, ruby ? null}:


Conventionally, the comma goes at the start of the line, and then the } on a line on its own:

{ foo, bar , baz }:

Thanks for your review. I'll be away for a week and will get back on this then.

guillaumecherel · 2020-08-21T14:26:52Z

Thanks to you for your careful review and advices.

As you suggested, I switched back to wrapping only the executable mcom for now, removed the EDITOR variable setting, removed the dependencies on less and nano and made the little formatting request. Let me know about anything else.

zowoq · 2020-08-22T01:23:12Z

@ofborg eval

guillaumecherel · 2020-08-25T11:08:30Z

I realized that the wrapping the script mcom broke the behaviours of the symlinks to it mrep, mbnc and mfwd. The reason is that mcom's behaviour depends on the value of $0, which normally changes when mcom is called through one of its symlink. The wrapper broke this. I wrapped mcom and its symlinks to fix this.

alyssais · 2020-08-27T10:04:41Z

pkgs/applications/networking/mailreaders/mblaze/default.nix

+  #     url = "https://github.com/leahneukirchen/mblaze/commit/53151f4f890f302291eb8d3375dec4f8ecb66ed7.patch";
+  #     sha256 = "1mcyrh053iiyzdhgm09g5h3a77np496whnc7jr4agpk1nkbcpfxc";
+  #   })
+  # ];


This should be deleted rather than commented out.

alyssais · 2020-08-27T10:05:29Z

pkgs/applications/networking/mailreaders/mblaze/default.nix

+    # designed to be run directly or via symlinks such as mrep. Using
+    # symlinks changes the value of $0 in the script, and makes it
+    # behave differently. When using the wrapProgram tool, the resulting
+    # wrapper breaks this behaviour. The following wrappers preserve it.


Interesting! The argv0 option to wrapProgram doesn’t fix this?

I've learned it doesn't work for scripts with shebangs. It does if you remove the shebang, but then you have to explicitly pass it to bash everytime you want to run it…

guillaumecherel · 2020-11-25T09:13:26Z

The master branch now has mblaze updated to version 1.0. Closing this pull request.

justinlovinger · 2020-12-03T21:35:33Z

@guillaumecherel It looks like mcom still needs file and other binaries wrapped in mblaze 1.0. Would you like to merge the mblaze 1.0 update and re-open this pull request (or open a new pull request to fix mcom off of the latest nixpkgs)?

guillaumecherel · 2020-12-04T10:06:43Z

Indeed, there it is!

guillaumecherel · 2021-01-19T13:11:47Z

It seems like this pull request has stalled even though I believe made the requested changes. Is there anything else to do on my part?

pkgs/applications/networking/mailreaders/mblaze/default.nix

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

justinlovinger · 2021-05-31T00:24:40Z

@SuperSandro2000 Does this look good to you now? It would be very nice to have a fully working mblaze.

github-actions · 2021-05-31T01:01:55Z

Successfully created backport PR #125015 for release-21.05.

ofborg bot requested a review from ajgrf August 7, 2020 17:59

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Aug 7, 2020

alyssais reviewed Aug 8, 2020

View reviewed changes

pkgs/applications/networking/mailreaders/mblaze/default.nix Outdated Show resolved Hide resolved

alyssais reviewed Aug 8, 2020

View reviewed changes

guillaumecherel force-pushed the fix/mblaze-needs-file branch from d3e2753 to 24af810 Compare August 9, 2020 13:46

guillaumecherel force-pushed the fix/mblaze-needs-file branch from 24af810 to 18d72cf Compare August 9, 2020 14:05

alyssais requested changes Aug 11, 2020

View reviewed changes

guillaumecherel force-pushed the fix/mblaze-needs-file branch 2 times, most recently from a9c7461 to b5796f4 Compare August 21, 2020 14:24

guillaumecherel force-pushed the fix/mblaze-needs-file branch from b5796f4 to f26d698 Compare August 25, 2020 11:04

alyssais reviewed Aug 27, 2020

View reviewed changes

guillaumecherel force-pushed the fix/mblaze-needs-file branch from f26d698 to b2a39ce Compare August 27, 2020 14:32

guillaumecherel closed this Nov 25, 2020

Wrap mcom to add dependencies to PATH.

6dde143

guillaumecherel reopened this Dec 4, 2020

ofborg bot added the 2.status: merge conflict label Dec 4, 2020

guillaumecherel force-pushed the fix/mblaze-needs-file branch from b2a39ce to 6dde143 Compare December 4, 2020 12:16

ofborg bot removed the 2.status: merge conflict label Dec 4, 2020

guillaumecherel requested a review from alyssais January 19, 2021 13:12

SuperSandro2000 requested changes Mar 21, 2021

View reviewed changes

pkgs/applications/networking/mailreaders/mblaze/default.nix Outdated Show resolved Hide resolved

pkgs/applications/networking/mailreaders/mblaze/default.nix Outdated Show resolved Hide resolved

guillaumecherel and others added 2 commits March 22, 2021 09:34

Update pkgs/applications/networking/mailreaders/mblaze/default.nix

e9d4b68

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

Update pkgs/applications/networking/mailreaders/mblaze/default.nix

1e777e5

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

SuperSandro2000 added the backport release-21.05 label May 31, 2021

SuperSandro2000 merged commit 34c6163 into NixOS:master May 31, 2021

github-actions bot mentioned this pull request May 31, 2021

[Backport release-21.05] mblaze: fix mcom to use file utility. #125015

Merged

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mblaze: fix mcom to use file utility. #94881

mblaze: fix mcom to use file utility. #94881

guillaumecherel commented Aug 7, 2020

guillaumecherel commented Aug 8, 2020

alyssais left a comment

guillaumecherel commented Aug 8, 2020

alyssais left a comment

alyssais Aug 8, 2020

alyssais Aug 8, 2020

guillaumecherel commented Aug 9, 2020

guillaumecherel commented Aug 9, 2020

alyssais left a comment

alyssais Aug 11, 2020

alyssais Aug 11, 2020

alyssais Aug 11, 2020

guillaumecherel Aug 11, 2020

guillaumecherel commented Aug 21, 2020

zowoq commented Aug 22, 2020

guillaumecherel commented Aug 25, 2020

alyssais Aug 27, 2020

alyssais Aug 27, 2020

guillaumecherel Aug 27, 2020

guillaumecherel commented Nov 25, 2020

justinlovinger commented Dec 3, 2020

guillaumecherel commented Dec 4, 2020

guillaumecherel commented Jan 19, 2021

justinlovinger commented May 31, 2021

github-actions bot commented May 31, 2021

		@@ -1,11 +1,13 @@
		{ stdenv, lib, fetchFromGitHub, fetchpatch, libiconv, ruby ? null }:
		{ stdenv, lib, fetchFromGitHub, fetchpatch, libiconv, ruby ? null, makeWrapper, coreutils, file, gawk, gnugrep, gnused }:

mblaze: fix mcom to use file utility. #94881

mblaze: fix mcom to use file utility. #94881

Conversation

guillaumecherel commented Aug 7, 2020

Motivation for this change

Things done

guillaumecherel commented Aug 8, 2020

alyssais left a comment

Choose a reason for hiding this comment

guillaumecherel commented Aug 8, 2020

alyssais left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guillaumecherel commented Aug 9, 2020

guillaumecherel commented Aug 9, 2020

alyssais 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

guillaumecherel commented Aug 21, 2020

zowoq commented Aug 22, 2020

guillaumecherel commented Aug 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guillaumecherel commented Nov 25, 2020

justinlovinger commented Dec 3, 2020

guillaumecherel commented Dec 4, 2020

guillaumecherel commented Jan 19, 2021

justinlovinger commented May 31, 2021

github-actions bot commented May 31, 2021