Page MenuHomeFreeBSD

mail/mailutils: Update to 3.8; fix circular dependency
ClosedPublic

Authored by jrm on Nov 13 2019, 8:11 PM.

Details

Summary
  • Remove the build dependency on Emacs since the installed elisp file is simple and does not need to be byte compiled.
  • Do not forget to add Phab URL when committing.

PR: 235890
Submitted by:
Reported by:

Test Plan

poudriere tetsport for 11/12 i386/amd64 look ok

Diff Detail

Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27496
Build 25728: arc lint + arc unit

Event Timeline

Zeus does not have Phabricator account AFAIK, he's one of those old school maintainers. Is there a PR for this, or just this differential? The patch is quite noisy because it contains lots of unrelated and needless changes (formatting, whitespace, etc.) but I'll take a closer look into Emacs/Python stuff you've added and update the port.

That one is known (about circular dependency) , I was thinking maybe there's another PR for version update, but didn't find one. It's OK.

I could have separated out the formatting changes, but it's mostly formatting change, which I don't think are needless.

Is there a PR for this, or just this differential?

Zeus is CCed on the PR and I linked to this review there, so hopefully he will find this.

The patch is quite noisy because it contains lots of unrelated and needless changes (formatting, whitespace, etc.) but...

It's mostly formatting changes, so I do not think it's so difficult to distinguish the updates, fixes and the formatting changes. I also do not think the formatting changes are needless. tobik@ has made some nice tools to standardize the formatting to give us consistency.

I'll take a closer look into Emacs/Python stuff you've added and update the port.

You will update the port? Why? You are not the maintainer and I spent hours debugging different issues to come up with these changes.

@jrm wrote:

I could have separated out the formatting changes, but it's mostly formatting change, which I don't think are needless.

They are non-functional, pessimize readability, and just gratuitous (why reformat MASTER_SITES and PLIST_SUB, why move GNU_CONFIGURE and shuffle around and and collapse the options' blocks?).

You will update the port? Why? You are not the maintainer and I spent hours debugging different issues to come up with these changes.

I've been doing most technical work on the port (and committing to it, as can be seen per svn log), while Zeus is actually using it actively in production and is our liaison contact with upstream author. If you insist on committing what you propose yourself I won't object, but I still prefer to leave out any formatting changes out of the diff and concentrate specifically on version update, Emacs and Python options. Everything else with the port's Makefile is fine as is.

Remove formatting changes. They can be added in a separated commit.

Add a few more formatting reversions that were missed. Update commit message.

jrm retitled this revision from mail/mailutils: Update to 3.8; formatting; fix circular dependency to mail/mailutils: Update to 3.8; fix circular dependency.Nov 14 2019, 3:02 AM
jrm edited the summary of this revision. (Show Details)
jrm edited the test plan for this revision. (Show Details)
jrm added a reviewer: danfe.
@jrm wrote:

Remove formatting changes.

Thanks. See how much better and clear it is now? :-) I've pinged Zeus in Jabber, he should be able to review and provide his feedback soon.

They can be added in a separated commit.

Again, there's no need for that.

rfyu28uyeg_snkmail.com added inline comments.
mail/mailutils/Makefile
44–45

First of all, I think EMACS_USES=emacs:noflavors with EMACS_NO_DEPENDS=yes could just be omitted (I may be missing a subtlety).

But even with EMACS_USES=emacs:noflavors and EMACS_NO_DEPENDS=yes, there's still no build dependency on emacs. So if no emacs is not installed at build time the .elc will not get built and 'check-plist' will fail with:

====> Checking for pkg-plist issues (check-plist)
===> Parsing plist
===> Checking for items in STAGEDIR missing from pkg-plist
===> Checking for items in pkg-plist which are not in STAGEDIR
Error: Missing: %%EMACS_SITE_LISPDIR%%/mailutils-mh.elc
===> Error: Plist issues found.
*** Error code 1

It would also fail with 'missing mailutils-mh.el' as well except for the do-install-EMACS-on.

Maybe it'd be best to just remove the EMACS_USES and EMACS_NO_DEPENDS, leave the do-install-EMACS-on to manually install the .el file. And then remove the .elc from the pkg-plist. Have a byte compiled version of the tiny mailutils-mh.el doesn't buy much anyway.

I think just manually installing the .el file is the way to go. In fact (maybe), why not just get rid of the EMACS option altogether and just always install the .el file if MH is on. Why bother over-optimizing OPTIONS for the sake of a small .el file? Just always install it with the MH option (and remove the .elc file in case it gets installed - just to make pkg-plist easy to manage) and be done. I guess that'd be my vote.

  • John
mail/mailutils/Makefile
44–45

The EMACS_USES=emacs:noflavors and EMACS_NO_DEPENDS=yes is there for EMACS_SITE_LISPDIR. Removing the build dependency on Emacs was intentional. It fixes the the circular dependency with the Emacs ports and mail/mailutils depending on each other.

There is no plist error now, because pkg-plist has been updated.

@danfe added the Emacs option, so I will remove it and always install the elisp file if he agrees.

mail/mailutils/Makefile
44–45

Got it now (about getting EMACS_SITE_LISPDIR).

The .elc might still be installed if emacs is installed (and thus detected) at configure / build time.

To solve that, either rm -f the .elc or pass in:

MH_CONFIGURE_ARGS= ac_cv_prog_EMACS=no

(I'm assuming here that we're getting rid of the now nearly pointless EMACS option and using MH_* instead of EMACS_*. But if for some reason, @danfe wants to keep EMACS, use 'EMACS_CONFIGURE_ARGS= ac_cv_prog_EMACS=no' instead).

Ensure that compiled elisp is not installed when Emacs is auto-detected.

mail/mailutils/Makefile
44–45

Good call with Emacs auto-detection. I forget that some are so barbarous :P to build packages in a live environment and not in a clean jail that poudriere provides.

As for the Emacs option, we may be underestimating the disdain some have for the editor of the gods. "I don't want this 1 KB Emacs lisp file polluting my system."

I added a patch based on Joseph's patch here in the PR. It only addresses the EMACS circular dependency issue and doesn't include the update to 3.8. I attached the patch there instead of here to avoid confusion. Maybe there's a good way to present an alternate patchset on the phab page, but I don't know how.

Anyway the patch there removes consolidates the EMACS option (which is only used for installing the mailutils-mh.el) under the MH option, thus simplifying the OPTIONS list. And it fixes the emacs<->mailutils circular dependency (works whether emacs is installed or not at build time). Joseph did the heavy lifting for that - I just tweaked it.

mail/mailutils/Makefile
44–45

I'm all for removing the option if it does not pull any dependencies; 1KB files should be installed unconditionally. In general, less options => better. :-)

It was only introduced initially because I've assumed that Emacs is needed for some reason, but since I'm not an expert on those things, I might have easily guessed wrong.

Collapse EMACS option into MH option

mail/mailutils/Makefile
44

Typically, ac_cv_* bits are passed via CONFIGURE_ENV (ARGS also work, but ENV is probably more correct and what is used in majority of other ports).

Zeus pointed out that there is a test suite which is not hooked to our framework. It would be nice if you could add TEST_TARGET=check so that the standard make test would work.

CONFIGURE_ARGS -> CONFIGURE_ENV
Add TEST_TARGET=check

All but one test passes.

sorry for delay

there is no objection against the diff

but for now, there is an issue with mu tests which should be solved first

it's better to wait it's been solved, before applying the diff

but for now, there is an issue with mu tests which should be solved first
it's better to wait it's been solved, before applying the diff

Tests should pass fine now after rP518222; @jrm please rebase your patch, and I believe it's good to go.

Add missing test dependency

The test fails without TEST_DEPENDS=automake:devel/automake

but for now, there is an issue with mu tests which should be solved first
it's better to wait it's been solved, before applying the diff

Tests should pass fine now after rP518222; @jrm please rebase your patch, and I believe it's good to go.

I guess you tested with autom4te installed. In a clean environment, the test will fail otherwise.

@jrm wrote:

I guess you tested with autom4te installed. In a clean environment, the test will fail otherwise.

Yes, in an unclean environment, that's right. I was more concerned with the functional breakage rather than what particular dependencies those tests might require which I trusted you'd figure out yourself. :-)

My tinderbox is currently a bit broken WRT running tests, I hope to look into this issue soonish.

This revision is now accepted and ready to land.Nov 23 2019, 3:59 PM

If there are no complaints, I will commit this within 24 hours.