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.

Sign up for GitHub

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

Jump to bottom

[RFC/RDY] neomutt: 2017-12-08 -> 2017-12-15 #32546

Closed
wants to merge 1 commit into from
Closed

[RFC/RDY] neomutt: 2017-12-08 -> 2017-12-15 #32546

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 10, 2017 •
edited by ghost

Motivation for this change

The new version contains two important bugfixes and drops the autotools build system.

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
    • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@ghost ghost mentioned this pull request Dec 10, 2017
Closed
8 tasks
@andir
Copy link
Member

andir commented Dec 11, 2017

@toogley The build still succeeds without the -lidn line but I get this (again):

/nix/store/519cp7axvh8s6d8bxwbif7ki09a422kb-neomutt-20171208/bin/neomutt: error while loading shared libraries: libidn.so.11: cannot open shared object file: No such file or directory

The patch that I provided to neomutt to fix that specific issue was merged but only affects the old autoconfigure script.

@ghost
Copy link
Author

ghost commented Dec 11, 2017 •
edited by ghost

@andir which patch? NIX_LDFLAGS = "-lidn" ? i haven't found any neomutt related commits in https://github.com/NixOS/nixpkgs/commits?author=andir

@andir
Copy link
Member

andir commented Dec 11, 2017

The PR was with neomutt: neomutt/neomutt#771

This was referenced Dec 11, 2017
Merged
Closed
@ghost
Copy link
Author

ghost commented Dec 11, 2017

@andir I cannot reproduce your error. I did nix-build -A neomutt --check ~/src/nixpkgs > .neomutt-idn-testbuild.log 2>&1 and the only thing containing error is:

error: derivation ‘/nix/store/2slc85kf64dc3qds3lfa61ch7msfnr2n-neomutt-20171208.drv’ may not be deterministic: output ‘/nix/store/519cp7axvh8s6d8bxwbif7ki09a422kb-neomutt-20171208’ differs

@andir
Copy link
Member

andir commented Dec 11, 2017

Did you execute neomutt?

@ghost
Copy link
Author

ghost commented Dec 11, 2017

@andir ah, i see. I can now reproduce your problem.

@ghost
Copy link
Author

ghost commented Dec 14, 2017 •
edited by ghost

I have added now nullmailer as optional dependency and tested it. autosetup finds the sendmail binary, when i set withSentmail to true by default. That means, "ac_cv_path_SENDMAIL=sendmail" (from 8d46191) is not necessary anymore.

Additionally, i have now added lua as an optional dependency and tested if it occurs in neomutt -v

I think this PR can be merged now.

@ghost ghost changed the title neomutt: 2017-10-27 -> 2017-12-08 [RFC] neomutt: 2017-10-27 -> 2017-12-08 Dec 14, 2017
@ghost ghost changed the title [RFC] neomutt: 2017-10-27 -> 2017-12-08 [RFC/RDY] neomutt: 2017-10-27 -> 2017-12-08 Dec 14, 2017
andir
andir reviewed Dec 14, 2017
View reviewed changes
pkgs/applications/networking/mailreaders/neomutt/default.nix Outdated
@@ -1,6 +1,10 @@
{ stdenv, fetchFromGitHub, which, autoreconfHook, makeWrapper, writeScript,
ncurses, perl , cyrus_sasl, gss, gpgme, kerberos, libidn, notmuch, openssl,
lmdb, libxslt, docbook_xsl, docbook_xml_dtd_42, mime-types }:
lmdb, libxslt, docbook_xsl, docbook_xml_dtd_42, mime-types
, withSendmail ? false, nullmailer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing ,withFeature ? false, feature you could just do: feature ? null.

Since null values are ignored within buildInputs you could also simplify the expression there.

Copy link
Author

@ghost ghost Dec 15, 2017 •
edited by ghost

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean like bc889b9 ?, i.e. its not possible to simplify the lua feature?

@ghost ghost changed the title [RFC/RDY] neomutt: 2017-10-27 -> 2017-12-08 [RFC/RDY] neomutt: 2017-12-08 -> 2017-12-15 Dec 15, 2017
neomutt: 2017-12-08 -> 2017-12-15
daf0293 
* add nullmailer as optional dependency
* add lua as optional dependency
orivej
orivej reviewed Dec 23, 2017
View reviewed changes
pkgs/applications/networking/mailreaders/neomutt/default.nix
mime-types
];
nullmailer # default to null, so its optional.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default is with nullmailer. nullmailer ? null only tells the users reading this file that it is supported to override nullmailer with null.

I don't see why this dependency should be disabled by default, only the comment is confusing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nullmailer ? null only tells the users reading this file that it is supported to override nullmailer with null.

Does that mean you think +, withSendmail ? true, nullmailer would be more readable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use

+, sendmail ? nullmailer, nullmailer ? null

+ buildInputs = [ … sendmail … ]

Then the users can disable it by overriding with sendmail = null, and choose another sendmail with e.g. sendmail = msmtp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW sendmail is provided by busybox, exim, msmtp, nullmailer, postfix, and ssmtp. Have you tested sending mail from neomutt with nullmailer?

@xeji
Copy link
Contributor

xeji commented May 14, 2018

closing as outdated, we're on a newer version already.

@xeji xeji closed this May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@andir @xeji @orivej @GrahamcOfBorg