Bug 204128

Created attachment 162569 [details]
UIDs/GIDs diff.

Created attachment 162570 [details]
SHAR File

Removed the .git directory.

Created attachment 162593 [details]
SHAR File

Updated with a more descriptive pkg-message.in file.

Comment 4 Kubilay Kocak 2015-10-30 09:36:33 UTC

Nice work David!

If and when you're happy with the version of the attachment that has been submitted, please set the maintainer-approval flag to (+) on those attachments that you would like a committer to look at.

Please also confirm that this passes QA [1], and if not, obsolete the existing attachments after updating them.

[1] https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/testing.html

Hi,

Thank you. I've went through the testing section again on the Porters Handbook and also tested the port using Poudiere. All appears to be well. A couple of WARNs on Portlint, but I hope not show-stoppers.

$ portlint -a
WARN: Makefile: [52]: possible use of "${CHMOD}" found. Use @(owner,group,mode) syntax or @owner/@group operators in pkg-plist instead.
WARN: Makefile: no ftp/http mirror in MASTER_SITES for users behind a proxy.
0 fatal errors and 2 warnings found.

-=david=-

Comment 6 Kubilay Kocak 2015-10-30 10:27:22 UTC

Thanks for the feedback David :)

Comment 7 Brad Davis 2015-11-02 14:30:44 UTC

Why not remove the ${CHMOD} and use @mode in the pkg-plist?

Hi Brad,

Thank you for your feedback. I've updated the pkg-plist to use @mode and removed the ${CHMOD} from the Makefile. Please retest.

Thank you.

-=david=-

Created attachment 162713 [details]
SHAR file

Comment 10 Thomas Zander 2015-11-07 08:09:42 UTC

Can we use the most recent 4.2.2 version instead?

Hi!

I don't see why not :-) It was just released. I'll update the port, do a test locally, then push up a new SHAR file.

Thank you kindly for looking :-)

-=david=-

Created attachment 162873 [details]
SHAR File

Update to Artifactory 4.2.2.

-=david=-

Comment 13 Thomas Zander 2015-11-07 08:40:58 UTC

(In reply to David Harrigan from comment #12)

Thanks for the quick response. Always happy to use the latest and hopefully greatest version.

Let's quickly have a look at pkg-plist. Why do all the files and dirs belong to artifactory:artifactory? Normally all file and dir permissions should be default with only dirs and files that the artifactory processes need to write having different permissions (log files, status files, etc.).

Hi,

I found that when I booted Artifactory (when the /usr/local/artifactory directory was owned by root), it failed since the Artifactory user could not write into the directory and thus create all the files/directories it requires. 

I suppose I could have made the directory group writeable and put the group ownership of the directory to "artifactory". I believe, given that inside the Artifactory directory there are only a few directories and files it was simply easier to let them all belong to Artifactory. However, I'm happy to play around with a few other ways of doing this based on what you can suggest? :-)

-=david=-

Comment 15 Thomas Zander 2015-11-07 10:57:55 UTC

(In reply to David Harrigan from comment #14)

In general you should always try to use the @marcos as sparingly as possible. Granted, sometimes they are needed, and sometimes apps will refuse to run properly if they can't have it their way, and in those cases we should apply them to make it work.
But it should be really really really not necessary for something like
%%APP_HOME%%/COPYING
to be owned and writable by the application. Similar goes for %%APP_HOME%%/etc/artifactory.config.xml.sample. Why should the application modify this file ever?

For this port, pkg-plist is fairly small, it should be relatively quick to find the minimum set of dirs and files that require non-default owner, group or modes.

I don't know whether you are using poudriere, but you should. It helps a lot for issues like this.

Comment 16 Thomas Zander 2015-11-07 11:13:20 UTC

Created attachment 162875 [details]
Content diff of %%APP_HOME%% before and after starting artifactory for the first time

Comment 17 Thomas Zander 2015-11-07 11:38:05 UTC

(In reply to Thomas Zander from comment #16)

Have a look at attachment 162875 [details] . I made a diff of the %%APP_HOME%% dirs and subdirs before and after starting artifactory using the default config for the first time.
It creates lots of files in data, logs and tomcat/webapps and changes etc. All those files would be leftovers in LOCALBASE. This should not happen.

I would split this into two parts:
1) The 'static' part of artifactory (i.e. files that we only need to read and execute, but never touch or write) should go to what is currently APP_HOME in your port. Nothing there should change after installation, such that it can be cleared perfectly by pkg delete.
2) The 'dynamic part' (i.e. logs, pidfiles, tomcat webapps and so on, everything that changes at runtime) should go to /var. pidfile into /var/run, the rest into an artifactory-owned subdir in /var. Those dirs must be registered with the @dir macro in plist. Since this changes at runtime, there will be leftover files after deinstall. There must be a pkg uninstall message informing the user which dirs may need manual cleanup.

Could you take a look?

Hi,

This is great feedback. I'll start looking at the issues and see what I can do to move this port on to the stage where it is ready!

Thank you very much :-)

-=david=-

Hi,

Question:

Should "var/artifactory" be "/var/artifactory" or "/usr/local/var/artifactory" (and same question for var/run?)?

Thank you.

-=david=-

Created attachment 162920 [details]
SHAR File

Hi,

Based upon recent feedback, here is an improved installation of Artifactory. I've tried to keep things separate, in their own /var/artifactory directory where appropriate. 

Artifactory does create some temporary files in /usr/local/artifactory (tomcat compiled files), but where done, I've clearly indicated in a deinstallation message which directories to clean up.

Please review and let me know if I need to improve upon this further :-)

Thank you.

-=david=-

Created attachment 162922 [details]
SHAR File

Further improvements. Hopefully good to go now! :-)

-=david=-

Please note, the new attachment, SHAR file, correctly creates tomcat 'var' temporary directories, thus allowing a clean removal of Artifactory.

Created attachment 162923 [details]
UID/GID for Artifactory

Brings this up-to-date with latest changes made elsewhere to UIDs/GIDs.

-=david=-

Comment 24 Thomas Zander 2015-11-09 17:18:26 UTC

(In reply to David Harrigan from comment #22)

Are you sure you uploaded the right attachments? Just a superficial look:
- PORTVERSION is 4.2.0 instead of 4.2.2
- owner macros in plist are still never reset
- PID file goes into ${APP_HOME}/bin/${PORTNAME}.pid
- APP_HOME expands to full path in plist expansion (which it should only do outside PREFIX)
- superfluous @dir entries in plist
- The shar does not contain anything with 'var' at all (except rcvar= ...)
- the gid/uid diff does not patch correctly to the latest changes

Could you please double check?

Created attachment 162928 [details]
SHAR File

Hi,

Terribly sorry for that! I forgot to redirect the contents of shar into a file, so when I generated the shar file, it was going to console...then I was uploading the old shar file!!!

d'oh!

-=david=-

Comment 26 Thomas Zander 2015-11-09 19:26:50 UTC

Created attachment 162936 [details]
Proposed update for artifactory shar (untested!)

I have made a few proposed changes, please review:
- Remove superfluous (I believe) files
- Reduce, simplify plist
- Use variable substitution for several variables that otherwise would have to be manually sync'ed over multiple files
- If I am not mistaken, there are multiple components of different LICENSE. Adapt LICENSE* accordingly
- Use lowercase spelling for shorter loop-variable, so it is clearer what the invariants and loop variables are

Please compare this with your most recent submission to see the differences.

Note that I have not *tested* whether the service actually *runs* flawlessly. You might want to double-check running it and tweak the shar until you are satisfied.

Created attachment 162937 [details]
SHAR File

Hi,

Attached is the updated SHAR file. It looks a lot better now thank you Thomas - just one thing, there was an error - it depends upon artifactory.default which you had removed in the updated Makefile. A quick test on Poudriere discovered that.

I've readded artifactory.default back in, tested and it appears to boot up successfully :-)

-=david=-

Comment 28 commit-hook 2015-11-09 20:32:14 UTC

A commit references this bug:

Author: riggs
Date: Mon Nov  9 20:31:42 UTC 2015
New revision: 401165
URL: https://svnweb.freebsd.org/changeset/ports/401165

Log:
  Add new port: devel/artifactory, a universal artifact repository manager

  PR:		204128
  Submitted by:	dharrigan@gmail.com (maintainer)

Changes:
  head/GIDs
  head/UIDs
  head/devel/Makefile
  head/devel/artifactory/
  head/devel/artifactory/Makefile
  head/devel/artifactory/distinfo
  head/devel/artifactory/files/
  head/devel/artifactory/files/artifactory.in
  head/devel/artifactory/files/pkg-message.in
  head/devel/artifactory/pkg-descr
  head/devel/artifactory/pkg-plist

Comment 29 Thomas Zander 2015-11-09 20:38:37 UTC

Here you go! Committed, thanks!
Have fun maintaining! :)

Thank you! 

I look forward to the challenges that brings :-)

Thank you kindly for your help and support in getting this out. I appreciate it!

-=david=-