Created attachment 148743 [details] sublime3 changes!
The pkg-plist changes are invalid. @dir is not a new name for @dirrm Please review /usr/ports/CHANGES for the proper way to use this new keyword. In most cases, @dirrm and @dirrmtry are simply removed, not replaced.
thanks for the clarification, this was actually generated by makeplist, but in that case the @dir part can be removed, should I file a new patch removing those changes?
(In reply to miguelmclara from comment #2) > thanks for the clarification, this was actually generated by makeplist, but > in that case the @dir part can be removed, should I file a new patch > removing those changes? It is considered user error to use the output of makeplist without editing it. *ESPECIALLY* with regards to directory removal. That is why the first list says "don't use this". Yes, please fix the pkg-plist. don't forget the makeplist makes other errors commonly, like adding substitutions of variable that don't belong. Review every line of it.
Created attachment 148945 [details] correct pkg-plist + removed @dir(s) + also: %%DATADIR%%/sublime_text.desktop.orig shouldn't be there, everything else was checked manually and looks ok!
okay, now please submit an updated patch for sublime3 that replaces these two attachments. Yes, I know you could apply the patch and then overwrite the plist with the second attachment but people like single patches to apply with "svn patch" and it's easier for review as well.
(In reply to John Marino from comment #5) > okay, now please submit an updated patch for sublime3 that replaces these > two attachments. > > Yes, I know you could apply the patch and then overwrite the plist with the > second attachment but people like single patches to apply with "svn patch" > and it's easier for review as well. Yes my fault should have tough of that in the first place, I can't do it right now, but will do tomorrow. Tks
Created attachment 149095 [details] full diff with "svn diff" This new patch should be enough for the changes to the Makefile and pkg-plist and was generated with svn diff, so it should be easy to use it svn patch I hope :) Also I was testing this in FreeBSD 10.1-PRERELEASE and sublime won't open... I don't see a segfault or anything in dmesg, I'll need to do some debugging, but it seems the port is broken on 10.1 (maybe do to recent changes in linuxolator, but not sure).
Ok found the problem... shm is needed, I have "link /tmp shm" in my 11-CURRENT devfs.conf but not on the 10.1 ofc. :)
(In reply to miguelmclara from comment #8) > Ok found the problem... shm is needed, I have "link /tmp shm" in my > 11-CURRENT devfs.conf but not on the 10.1 ofc. > > :) About this... should I had this info to pkg-message? Or since the app doesn't really work would the best approach to test if "link /tmp shm" exists in devfs.conf and exit if not (I would patch the sublime.in for this ofc)!? Please advise! thanks
Created attachment 149785 [details] svn diff for port update - Fix desktop file and icons - Fix pkg-plist (remove unneeded files and and /Icons needed for the Title/taskbar) - add gtk2 dependency
Please verify this for upgrading the port, since its not approved yet I took the opportunity to add another fix, which includes: + Patch to the desktop file (adapt to freebsd, correct the bin path and remove unity stuff) The rest is the same already submitted in the previous patch (fix pkg-plist and makefile gtk2 dependency) The dev/shm is not really a port porblem since when we install linux-c6 we are warned about the need of shm and if we run: sublime -w it will error out (if shm is not linked it will inform the user) Hope I haven't missed anything this time :)
this looks okay, although there are no test logs to confirm. However, there's an existing issue with do-install target. e.g. "@${INSTALL_DATA} ${WRKSRC}/Icon/48x48/sublime-text.png ${STAGEDIR}${PREFIX}/share/icons/hicolor/48x48/apps/" You see that "@" in front of the command? That suppresses the output and we don't want output suppressed for install commands. (it is permissible on ${MKDIR} commands but not any others) At the very least, you need to provide a new patch that strips off all those "@" symbols from install (and pre/post-install if you use those). Also, provide the output of these commands: 1) portlint 2) make check-plist 3) make stage-qa thanks!
we cross posted but I don't see anything in your new post that would affect my comment.
Damn... I saw that on other linux ports (including the old sublime2 port) and tough I was supposed to do it :P Thanks for the warning and sorry for being a PITA but just to be sure should I remove them from the MKDIR commands too? I know you said "it is permissible on ${MKDIR}", but is the best way to not strip off those too?
(In reply to miguelmclara from comment #14) > Thanks for the warning and sorry for being a PITA but just to be sure should > I remove them from the MKDIR commands too? > I know you said "it is permissible on ${MKDIR}", but is the best way to not > strip off those too? Most people leave the "@" in front of ${MKDIR}. Personally I also combine mkdir commands, e.g. @${MKDIR} ${dir1} @${MKDIR} ${dir2} @${MKDIR} ${dir3} would change to : @${MKDIR} ${dir1} \ ${dir2} \ ${dir3} I usually put 1 directory per line for readability and I usually put mkdir as the first command. These are just style things I personally do, so other people do it too, but it's optional.
Thanks for the hint... Funny thing using vim to edit the make file I somehow got spaces after "do-install:" Using sublime to edit it fixed it. Probably related with tabspaces I guess... Anyway: portlint WARN: Makefile: [21]: possible direct use of command "strip" found. use ${STRIP_CMD} instead. WARN: Makefile: [46]: possible use of "${CHMOD}" found. Use @(owner,group,mode) syntax or @owner/@group operators in pkg-plist instead. WARN: Makefile: Consider defining LICENSE. 0 fatal errors and 3 warnings found. abotu [21] I'm using: STRIP= # don't strip linux binaries. Should I use STRIP_CMD? AS for CHMOD I'm really not sure what else to do... (using @mode in plist doesn't seem to work with pkg-static) check-plist ends in (full log: http://filebin.ca/1iNkVoZSGRnY): ====> Compressing man pages (compress-man) ====> 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 ===> No pkg-plist issues found (check-plist) make stage-qa ====> Running Q/A tests (stage-qa) Only output I get was that!
Created attachment 149789 [details] removed the @ directives
1) yes, remove STRIP=. I don't think it does anything (you don't build and you don't use INSTALL_PROGRAM). i bet if you remove the line it will make no difference. 2) set your tabs equal to 8 spaces -- lots of tabs aren't lining up 3) on the compound MKDIR command, tab the second and subsequent lines twice (one more tab than the first line) 4) for CHMOD, you can leave it based on how you copytree-share. not great but it's okay.
Created attachment 149794 [details] fixed tab spacing? I used "/usr/bin/vi Makefile" and redone the tabs, portlint doesn't complain at least but it wasn't after I edited with sublime to (I think sublime defaults to 4 btw) Is it ok now?
Created attachment 149795 [details] Correct Patch The previous one was missing the double tab space one the MKDIR lines!
Created attachment 149796 [details] Correct patch For real now!!! :|
To sum it up: - Fix desktop file and icons - add gtk2 dependency - Fix pkg-plist (remove unneeded files and and /Icons needed for the Title/taskbar) - Remove "STRIP" in the Makefile - Remove "@" where except for MKDIR in the Makefile
i don't know what editor you are using, but it's getting worse with respect to tabs. You changed actual tabs to spaces. Things like "USES=" that should have been tabbed to line up with the second tab wasn't. Can you look again? use TABS not spaces (maybe I confused you with 8 spaces, I meant one tab needs to be equivalent to 8 spaces)
That's super wired I used the systems vi (not vim) and used TAB not spaces :| Also portlint should complain about the tabs here... But the line you mention was already there... (copied from the port on xmj github repo) Let me try to go to every line again
i don't think portlint complains about that, and we review the entire port, not just changes (so that's why you are fixing things you didn't touch)
Created attachment 149797 [details] Correct Patch So, I did find spaces in several lines up in the Makefile... so it seems this lines were already "broken". I used vi and tabs only in all lines (double tab in USES and WRKSRCto line it up, if this is wrong I'll file a new one) But I don't see spaces anywhere now so hopefully this one is correct :)
If you increment PORTREVISION (or define it to 1 if it doesn't exist), I think if will be in great shape.
Created attachment 149800 [details] add POrTREVISION
thanks, I'm promoting this PR now.
Thank you for helping with all the corrections :)
A commit references this bug: Author: robak Date: Wed Nov 26 23:46:38 UTC 2014 New revision: 373471 URL: https://svnweb.freebsd.org/changeset/ports/373471 Log: editors/sublime3: various fixes - Fix desktop file and icons - Add gtk2 dependency - Fix pkg-plist (remove unneeded files and and /Icons needed for the Title/taskbar) - Remove "STRIP" in the Makefile - Remove "@" where except for MKDIR in the Makefile PR: 194669 Submitted by: Miguel Clara <miguelmclara@gmail.com> Changes: head/editors/sublime3/Makefile head/editors/sublime3/files/sublime.in head/editors/sublime3/pkg-plist
Committed, thanks for your work!
Sorry to bring this one up again I notice that files/patch-sublime_text.desktop is missing in the portstree so I wonder if I missed anything and if I should file a new report with just that change (and bump the portrevision ofc) In the svn diff the file is present, I'm not sure if in the reported I should actually mention svn status (maybe this is were I've failed): svn status A files/patch-sublime_text.desktop Please advise, Thanks
Hi, It seems the patch to the desktop file was never added, should I create a new PR?
new (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=197381) opened.