Opened 5 years ago

Closed 4 years ago

#47231 closed enhancement (fixed)

resolve minor nits with par

Reported by: grimreaper (Eitan Adler) Owned by: qbarnes (Quentin Barnes)
Priority: Low Milestone:
Component: ports Version: 2.3.3
Keywords: haspatch Cc: Ionic (Mihai Moldovan)
Port: par

Description


Attachments (2)

qbarnes.diff (907 bytes) - added by grimreaper (Eitan Adler) 5 years ago.
qbarnes.diff​ (692 bytes) - added by grimreaper (Eitan Adler) 5 years ago.

Download all attachments as: .zip

Change History (14)

Changed 5 years ago by grimreaper (Eitan Adler)

Attachment: qbarnes.diff added

comment:1 Changed 5 years ago by Ionic (Mihai Moldovan)

Cc: qbarnes@… removed
Owner: changed from macports-tickets@… to qbarnes@…
Priority: NormalLow

comment:2 Changed 5 years ago by qbarnes (Quentin Barnes)

Why is it a good idea to change the perms on the executable from 0555 to 0755?

comment:3 Changed 5 years ago by grimreaper (Eitan Adler)

this was part of a project to add consistency. I was told the 'correct' way was 0755 for my own ports and then just fixed it for all the others too.

comment:4 Changed 5 years ago by Ionic (Mihai Moldovan)

The default UNIX convention for binaries installed in bindir is 0755.

comment:5 Changed 5 years ago by larryv (Lawrence Velázquez)

This is correct and can be expressed by simply deleting “-m 555” (xinstall defaults to 0755), but it doesn’t merit a revbump.

Changed 5 years ago by grimreaper (Eitan Adler)

Attachment: qbarnes.diff​ added

comment:6 Changed 5 years ago by grimreaper (Eitan Adler)

updated diff to not have revbump and remove explicit -m 555

comment:7 Changed 5 years ago by qbarnes (Quentin Barnes)

Ah, ok. Nothing technically wrong with the way it was other than violating convention. It is a change to bring it into alignment with other packages. In that case, sounds good then, especially with removing the -m option letting xinstall default rather than rehardcoding a new value.

So not bumping the rev is simply to save triggering a rebuild for people for a inconsequential update?

If so, the patch looks good to me and should be applied. What's the next step? I don't have commit access.

comment:8 in reply to:  7 Changed 5 years ago by Ionic (Mihai Moldovan)

Replying to qbarnes@…:

Ah, ok. Nothing technically wrong with the way it was other than violating convention.

Yep, it's merely an enhancement.

It is a change to bring it into alignment with other packages.

The vast majority of packages either don't specify the mode or use 755. There are some ports currently installing with 555 for binaries, but that's what these tickets are for (or somesuch.)

In that case, sounds good then, especially with removing the -m option letting xinstall default rather than rehardcoding a new value.

So not bumping the rev is simply to save triggering a rebuild for people for a inconsequential update?

Well, it's for saving triggering a rebuild for people/users... mind you, that's "wrong" to my mind, as the filesystem and Portfile will end up diverging, even though this diversion is rather cosmetic and not influencing how a package works. I, personally, still prefer consistency over the comfort of not having to rebuild packages. Most other MacPorts developers seem to think otherwise, however, so I don't really care.

If so, the patch looks good to me and should be applied. What's the next step? I don't have commit access.

I can apply everything, as long as the maintainer gives explicit permission.

comment:9 Changed 4 years ago by casr+macports@…

Replying to ionic@…:

Replying to qbarnes@…:

If so, the patch looks good to me and should be applied. What's the next step? I don't have commit access.

I can apply everything, as long as the maintainer gives explicit permission.

qbarnes@ is the maintainer so I think that looks like permission to apply the patch :)

comment:10 Changed 4 years ago by qbarnes (Quentin Barnes)

Is there a reason this patch still hasn't be integrated?

Does something else I need to do to mark it as ready to get it in the queue?

comment:11 Changed 4 years ago by Ionic (Mihai Moldovan)

Yeah... actually, I waited for explicit permission, as said before. I'll take your latest comment as that and will apply the patch (in two steps.)

comment:12 Changed 4 years ago by Ionic (Mihai Moldovan)

Resolution: fixed
Status: newclosed

Modeline in r140221, file permission in r140222.

Thanks!

Note: See TracTickets for help on using tickets.