Bug 194669

Comment 1 John Marino 2014-11-01 15:08:05 UTC

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?

Comment 3 John Marino 2014-11-02 17:52:31 UTC

(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!

Comment 5 John Marino 2014-11-02 23:00:29 UTC

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 :)

Comment 12 John Marino 2014-11-24 18:40:38 UTC

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!

Comment 13 John Marino 2014-11-24 18:41:17 UTC

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?

Comment 15 John Marino 2014-11-24 18:57:15 UTC

(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

Comment 18 John Marino 2014-11-24 21:12:56 UTC

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

Comment 23 John Marino 2014-11-24 21:38:42 UTC

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

Comment 25 John Marino 2014-11-24 21:49:20 UTC

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 :)

Comment 27 John Marino 2014-11-24 22:02:33 UTC

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

Comment 29 John Marino 2014-11-24 22:10:04 UTC

thanks, I'm promoting this PR now.

Thank you for helping with all the corrections :)

Comment 31 commit-hook 2014-11-26 23:47:19 UTC

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

Comment 32 Bartek Rutkowski 2014-11-26 23:48:11 UTC

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.