- 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:
Details
Details
poudriere tetsport for 11/12 i386/amd64 look ok
Diff Detail
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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.
Comment Actions
The PR is listed in the description. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235890
Comment Actions
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.
Comment Actions
I could have separated out the formatting changes, but it's mostly formatting change, which I don't think are needless.
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.
Comment Actions
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.
Comment Actions
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.
| mail/mailutils/Makefile | ||
|---|---|---|
| 44 ↗ | (On Diff #64301) | 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.
|
| mail/mailutils/Makefile | ||
|---|---|---|
| 44 ↗ | (On Diff #64301) | 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 ↗ | (On Diff #64301) | 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). |
| mail/mailutils/Makefile | ||
|---|---|---|
| 44 ↗ | (On Diff #64301) | 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." |
Comment Actions
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 ↗ | (On Diff #64301) | 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. |
| mail/mailutils/Makefile | ||
|---|---|---|
| 53 ↗ | (On Diff #64417) | 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). |
Comment Actions
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.
Comment Actions
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
Comment Actions
Add missing test dependency
The test fails without TEST_DEPENDS=automake:devel/automake
Comment Actions
I guess you tested with autom4te installed. In a clean environment, the test will fail otherwise.
Comment Actions
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.