Created attachment 183366 [details] Diff for new/updated Makefile Makefiles of mail/alpine (and slave port editors/pico-alpine) needed some cleaning. Attached is the diff for the updated one for mail/alpine.
Comment on attachment 183366 [details] Diff for new/updated Makefile Change maintainer-approval, submitter is maintainer.
Comment on attachment 183366 [details] Diff for new/updated Makefile Please add maintainer approval.
Don't understand, how do I add maintainer approval?
(In reply to Marco Beishuizen from comment #3) Where the Attachments are, click Details. In the flag section, you should see maintainer-approval with a drop down box X, change to + and save
I found some things that may need to be addressed. ssl is currently an option to build into the port. The patch removes this optionand forces it to be compiled with ssl. Is this necessary? MAKE_JOBS_UNSAFE= yes Is this really required? The current ASPELL is the default. ISPELL is an alternative option to ASPELL This patch removed the ISPELL option. Is this intended? DOC Please check the DOC's changes, I don't think adding the post-install-DOCS-on target is working as you expect, but I could be wrong. LDAP_LIB_DEPENDS variable should be removed, this is automatically added with OPENLDAP_USE If a version is required, the variable DEFAULT_OPENLDAP_VER should be set. OPENLDAP_USE should be LDAP_OPENLDAP_USE otherwise it will always be enabled and not an option. The PASSFILE filename was changed, is this intended?
(In reply to Richard Gallamore from comment #5) - During the build I got the warning that the port needs USES+= ssl, so I added that. I also think that a mailer nowadays shouldn't be without SSL. - The MAKE_JOBS_UNSAFE=yes line was already there and just left it. Don't think it's really needed. - The ISPELL option in the old Makefile actually configured aspell, so I changed the option's name to the correct one. - Afaics the docs are installed in /usr/local/share/doc/alpine, just as before. So looks fine to me. - Without the LDAP_LIB_DEPENDS I got a build error about LDAP library dependencies not found. So I added this line to make the build work. - Will change OPENLDAP_USE to LDAP_OPENLDAP_USE. - In my quest of making Alpine save the passwords again, I at one point thought that the reason it didn't work was that the port used a non standard name for the file, so changed it to .alpine-passfile. Should this be changed to .alpine.pwd again? Thanks for the help Regards, Marco
(In reply to Marco Beishuizen from comment #6) >- In my quest of making Alpine save the passwords again, I at one point thought that the reason it didn't work was that the port used a non standard name for the file, so changed it to .alpine-passfile. Should this be changed to .alpine.pwd again? The change is fine, but it may break the password saving feature for some users. An update note in UPDATING will need to be added. Also adding a note in pkg-message would be helpful to users. Also, I missed PORTREVISION. Anytime the package needs to be regenerated that is not an official release, the PORTREVISION needs to be bumped up by one.
(In reply to Richard Gallamore from comment #7) I'll change the password file back to .alpine.pwd, saves some hassle. Didn't know a portrevision was necessary when only changing the Makefile. But will bump PORTREVISION too.
(In reply to Richard Gallamore from comment #5) MAKE_JOBS_UNSAFE=yes appears to be necessary, Alpine doesn't build without it.
Created attachment 183405 [details] mail/alpine diff after some corrections New diff after corrections: - PORTREVISION set to 1 - left MAKE_JOBS_UNSAFE in because of buildfail without it - left LDAP_LIB_DEPENDS in because of an error during build that Makefile needs LIB_DEPENDS set
Created attachment 183408 [details] alpine-2.21_to_alpine-2.21_1.diff (In reply to Marco Beishuizen from comment #10) > - left LDAP_LIB_DEPENDS in because of an error during build that Makefile needs LIB_DEPENDS set I think I may know why you were having issues ldap error. By default, alpine will check if ldap is installed and if so, will be enabled. To forcefully disable this check the --without-ldap compile time option must be used. This would only affect someone building in a dirty env, but please add this when configured off. This will probably also fix the error you are receiving when the LDAP_LIB_DEPENDS are not present. I also attached a diff comparing the build log between the changes, all the ssl configuration options were removed. Also, by default no spell checker is enabled, aspell was the default in previous releases. This is okay if intended but consider making it the default as previously.
(In reply to Richard Gallamore from comment #11) [I think I may know why you were having issues ldap error. By default, alpine will check if ldap is installed and if so, will be enabled. To forcefully disable this check the --without-ldap compile time option must be used. This would only affect someone building in a dirty env, but please add this when configured off. This will probably also fix the error you are receiving when the LDAP_LIB_DEPENDS are not present.] Doesn't the LDAP_CONFIGURE_WITH option take care of this? If the user leaves this unchecked, Alpine builds with --without-ldap.
(In reply to Richard Gallamore from comment #11) Ik think I've found a solution for the LDAP error. "LDAP_OPENLDAP_USE=" needs to be changed into "LDAP_USE=", and then the LDAP_LIB_DEPENDS lines can be removed.
Created attachment 183508 [details] mail/alpine diff 20170615 New diff including all remarks to date.
(In reply to Marco Beishuizen from comment #14) This is looking great! I found two items, ASPELL, and ssl. -ASPELL: During the configure stage if ASPELL option is enabled --with-interactive-spellcheck will appear twice because it is listed in both _WITH and _ON. Also, because there are really only two possible configuration options for spellcheck, its probably better to just remove the NOSPELL option and add it to ASPELL_CONFIGURE_OFF. -ssl: Because the USES= ssl option now supports multiple variants, eg base openssl, ports openssl, libressl and the -devel versions. The configuration variables that are removed will disable this feature to build on different ssl variants. Therefore I cannot commit this without them. The changes mentioned are minor, I'll fix them and start checking the pico- port. Thanks for taking the time to modernize the port!
Created attachment 183580 [details] mail/alpine.diff Here is my proposed changes. I have already ran QA and all tests are good. This is with the editors/pico-alpine port left untouched. If you approve of it I will go ahead and create the commit, if not then please submit your proposal and i'll start more tests.
Created attachment 183581 [details] mail/alpine.diff I forgot to remove the NOSPELL_desc, this patch removes it.
(In reply to Richard Gallamore from comment #16) Yes, I approve. And thanks a lot for all your help! Regards, Marco
(In reply to Marco Beishuizen from comment #18) Please set maintainer approval next to patchfile, Details -> maintainer-approval X -> + and submit. Thanks
Created attachment 183607 [details] mail/alpine.diff I acually made a mistake and removed all the options for pico-alpine. This patch fixes this.
A commit references this bug: Author: ultima Date: Mon Jun 19 17:39:21 UTC 2017 New revision: 443900 URL: https://svnweb.freebsd.org/changeset/ports/443900 Log: * Removed options SSL, ISPELL, NOSPELL * Added option ASPELL set as default * Modernized makefile PR: 219895 Submitted by: Marco Beishuizen <mbeis@xs4all.nl> (maintainer) Reviewed by: lifanov (mentor) Approved by: lifanov (mentor) Differential Revision: https://reviews.freebsd.org/D11263 Changes: head/mail/alpine/Makefile head/mail/alpine/pkg-descr
Committed, thanks!