submission: cpuid

[RJVB (René Bertin)]

This port provides a command line tool to query the cpuid instruction:

long_description    "cpuid" is a very simple C program, designed to dump and extract information \
                    from the x86 CPUID instruction. \n\
                    cpuid is capable of dumping all CPUID leaves (except any unknown leaves which \
                    require special ECX values to dump all information). cpuid can only decode \
                    certain leaves, but this functionality will be expanded as the CPUID \
                    specifications provided by AMD and Intel change.

[Ionic (Mihai Moldovan)]

Missing Id line.

This is not a license.

long_description messed up.

PortGroup should come (directly) after PortSystem. I'm surprised this even works.

${configure.ldflags}, ${configure.cppflags}, ${configure.cc} not respected.

This port has probably deserved being universal, too (note that use_configure no removes the implicit universal flag.)

Misses a dependency on libgnugetopt. Should not use the internal version => NO_GNU_GETOPT.

What the hell is CHUD? We probably don't want this either.

There's likely some other stuff I missed.

[RJVB (René Bertin)]

Replying to ionic@…:

Just a thing: this is (to be) an openmaintainer port, so anyone can feel free to make changes they see fit. I don't have commit permissions anyway.

Missing Id line.

This is a new Portfile written from scratch, not a fork of anything existing. If that line isn't generated automatically, what should I put on it?!

This is not a license.

There's only this to work with:

 * Copyright (c) 2010-2015, Steven Noonan <steven@uplinklabs.net>
 *
 * Permission to use, copy, modify, and/or distribute this software for any
 * purpose with or without fee is hereby granted, provided that the above
 * copyright notice and this permission notice appear in all copies.
 *
 * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
 * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
 * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
 * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
 * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
 * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

I have no idea if that's an existing license and how it's called if so. I could call it the cpuid license or I can remove the license line completely.

long_description messed up.

In what sense? Prints well enough here...

PortGroup should come (directly) after PortSystem. I'm surprised this even works.

Fortunately that's not true: there are many ports that include portgroups in variants or subports. That wouldn't be possible if PortGroup should come *directly* after PortSystem. I like to organise Portfiles by section, with everything related to fetching grouped together after name, version and metadata.

Misses a dependency on libgnugetopt. Should not use the internal version => NO_GNU_GETOPT.

It doesn't. It uses an internal version if NO_GNU_GETOPT is set, but I don't see any evidence that that's the case. Build as is, the binary doesn't depend on a getopt (or any other) library (and I'd prefer it that way).

What the hell is CHUD? We probably don't want this either.

CHUD is sadly RIP; it was used in Shark, and here to emulate pthread_setaffinity_np(). It's not being linked anyway because the test whether to link CHUD fails.

[Ionic (Mihai Moldovan)]

Replying to rjvbertin@…:

Replying to ionic@…:

Just a thing: this is (to be) an openmaintainer port, so anyone can feel free to make changes they see fit. I don't have commit permissions anyway.

Yes, but you're submitting a new port and we're reviewing it. Suggested changes should be applied by the submitter and the input is meant as a way of learning how "stuff is done", not to annoy anyone.

Missing Id line.

This is a new Portfile written from scratch, not a fork of anything existing. If that line isn't generated automatically, what should I put on it?!

# $Id$

If this line is set (and it really should be), it will be replaced by subversion on checkout with the "correct" long value (revision, author, time...)

This is not a license.

There's only this to work with: [...] I have no idea if that's an existing license and how it's called if so. I could call it the cpuid license or I can remove the license line completely.

If you had googled the first sentence ("Permission to use, copy [...]") you would have directly landed here: ​https://en.wikipedia.org/wiki/ISC_license

It's the ISC license.

long_description messed up.

In what sense? Prints well enough here...

There's at least one \n that shouldn't be there. We normally don't expect empty lines in long_description or description.

PortGroup should come (directly) after PortSystem. I'm surprised this even works.

Fortunately that's not true: there are many ports that include portgroups in variants or subports. That wouldn't be possible if PortGroup should come *directly* after PortSystem. I like to organise Portfiles by section, with everything related to fetching grouped together after name, version and metadata.

True, but the github PortGroup sets a few defaults which are meant to be overridden later. That's why it's preferred to be "included" early.

Misses a dependency on libgnugetopt. Should not use the internal version => NO_GNU_GETOPT.

It doesn't. It uses an internal version if NO_GNU_GETOPT is set, but I don't see any evidence that that's the case. Build as is, the binary doesn't depend on a getopt (or any other) library (and I'd prefer it that way).

Err, yes, NO_GNU_GETOPT shouldn't be set. Have you tested building this in trace mode? Note that OS X only ships BSD getopt and while that may even compile, there's no saying it behaves correctly at run time.

libgnugetopt should probably be added as a lib or build dependency, depending on how the software uses it exactly.

What the hell is CHUD? We probably don't want this either.

CHUD is sadly RIP; it was used in Shark, and here to emulate pthread_setaffinity_np(). It's not being linked anyway because the test whether to link CHUD fails.

Well, there isn't a "test" per-se, only a test whether the variable USE_CHUD is defined and not empty. Presumably that's okay for now, but this behavior might change at a later time and this is meant as a heads-up.

[RJVB (René Bertin)]

Replying to ionic@…:

the input is meant as a way of learning how "stuff is done", not to annoy anyone.

It may be just me, but I often find the tone of that input often bordering insult ... and to be really educational one would not let the pupil ask "how it's done" after being told "that's not how it's done" ;)

If you had googled the first sentence ("Permission to use, copy [...]") you would have directly landed here: ​https://en.wikipedia.org/wiki/ISC_license

The thought did occur to me, but that language is so common that I was sure to get hits from lots of comparable but not identical licenses.

There's at least one \n that shouldn't be there. We normally don't expect empty lines in long_description or description.

That's a \n, not a \n\n, so causes only a newline, not an empty line. I like to respect the formatting the author applied, as far as possible.

True, but the github PortGroup sets a few defaults which are meant to be overridden later. That's why it's preferred to be "included" early.

I'd have noticed if it had overridden any of my settings, or if something hadn't worked O:-)

Err, yes, NO_GNU_GETOPT shouldn't be set. Have you tested building this in trace mode?

I don't think I have trace mode in my version of base, but I shouldn't have to. No attempt is made to link to libraries from MacPorts. Note that OS X only ships BSD getopt and while that may even compile, there's no saying it behaves correctly at run time.

You think I didn't test the application before submitting the portfile? Of course it works. The code doesn't check for NO_GNU_GETOPT; the shipped version *is* BSD getopt which can apparently be used without build-time changes to the code. I don't think there's any point in forcing a link to libgnugetopt.

One of the applications of this utility that I see is including it with an installer (or other software) to check for CPU capabilities. Not depending on additional is an advantage in that kind of application.

[larryv (Lawrence Velázquez)]

Replying to rjvbertin@…:

Replying to ionic@…:

There's at least one \n that shouldn't be there. We normally don't expect empty lines in long_description or description.

That's a \n, not a \n\n, so causes only a newline, not an empty line. I like to respect the formatting the author applied, as far as possible.

The description is not meant to be formatted. A few ports do so, and it looks terrible. Don’t format it. If the text doesn’t make sense without being formatted, get new text.

True, but the github PortGroup sets a few defaults which are meant to be overridden later. That's why it's preferred to be "included" early.

I'd have noticed if it had overridden any of my settings, or if something hadn't worked O:-)

Just put it at the top of the portfile. Not doing so confuses the rest of us because we think that there must have been a good reason to do otherwise, and there isn’t one.

[Ionic (Mihai Moldovan)]

Replying to rjvbertin@…:

Replying to ionic@…:

the input is meant as a way of learning how "stuff is done", not to annoy anyone.

It may be just me, but I often find the tone of that input often bordering insult ... and to be really educational one would not let the pupil ask "how it's done" after being told "that's not how it's done" ;)

No, it's not meant as an insult. Yes, it can come off as harsh, but it's not meant to be that either. I've had lengthy discussions with other professionals when reviewing their patches and they assured me that my points of critique were spot-on and it's better to think about something twice then mess it up first and fix it later. I stopped constantly apologizing after pointing out it's just dry, not harsh or insulting.

If you had googled the first sentence ("Permission to use, copy [...]") you would have directly landed here: ​https://en.wikipedia.org/wiki/ISC_license

The thought did occur to me, but that language is so common that I was sure to get hits from lots of comparable but not identical licenses.

Yeah, I thought that too, but interestingly it lead right to the correct license - and I tried anyway, even with having that thought in mind. Even *if* multiple licenses were returned, I would have gone through them to find the correct one.

There's at least one \n that shouldn't be there. We normally don't expect empty lines in long_description or description.

That's a \n, not a \n\n, so causes only a newline, not an empty line.

Yes, you're right, sorry.

I like to respect the formatting the author applied, as far as possible.

See Larry's answer on that, we don't format these things.

I'd have noticed if it had overridden any of my settings, or if something hadn't worked O:-)

Also here.

I don't think I have trace mode in my version of base, but I shouldn't have to.

Base supports trace mode as of version 2.3.0. If you don't have that, you should really, really, really upgrade...

No attempt is made to link to libraries from MacPorts.

It's not just about libraries, but also headers and other stuff, which you will *not* see by looking at the compile output.

You think I didn't test the application before submitting the portfile?

No, but...

Of course it works.

I don't think you tested everything. Including old OS X versions, new ones, passing combinations of long, short options with and without parameters and invalid options.

Personally, I probably wouldn't test older versions either, because I don't have a quick way of doing so, but definitely the other stuff if I'm unsure about how the program would behave with getopt as shipped by Apple.

The code doesn't check for NO_GNU_GETOPT; the shipped version *is* BSD getopt which can apparently be used without build-time changes to the code. I don't think there's any point in forcing a link to libgnugetopt.

Okay, they are shipping NetBSD's getopt. That's probably compatible enough.

---

Regarding your new Portfile: looks better.

Please change the ID line to just

# $Id$

as already suggested (c.f., ​Section 4.2 of the MacPorts Development Guide.)

Also move it after any modeline.

I'd move github.setup up and remove the name and version variables. They are automatically set by github.setup.

I don't really like putting the replacement stuff into configure. It's really patching, not running a configure script or program. But as it's more or less indeed "configuring" the package, I guess we can leave it as-is.

However, there are two problems with that: because you used use_configure no, there won't be an implicit universal variant. Please add one via variant universal {} after use_configure no.

Secondly, you're way overcomplicating things.

Drop the variant_isset universal check.

reinplace -Os with ${configure.cppflags} ${configure.cflags} [get_canonical_archflags cc] and LDFLAGS := -lm with LDFLAGS := -lm ${configure.ldflags} [get_canonical_archflags ld]

Make sure to not drop -lm again!

Don't reinplace CC.

Rather, append CC=${configure.cc} LD=${configure.cc} to build.args (outside of configure of course.)

---

Edited for: ID line.

[RJVB (René Bertin)]

Replying to ionic@…:

I stopped constantly apologizing after pointing out it's just dry, not harsh or insulting.

I wasn't necessarily thinking of you. In your case I simply bounced the ball back into your field rather than trying to figure out if I should do more than simply include a stub Id line. Which turned out to be enough :)

Yeah, I thought that too, but interestingly it lead right to the correct license - and I tried anyway, even with having that thought in mind. Even *if* multiple licenses were returned, I would have gone through them to find the correct one.

Heh, and I was more or less convinced that someone on here would find the issue important enough maybe even to recognise the license blurb at first sight. Sorry it had to, correction, turned out to be you.

Base supports trace mode as of version 2.3.0. If you don't have that, you should really, really, really upgrade...

My lazy. I have 2.3.3 and too quickly assumed that the feature was still available in "development builds" only. Good to know, because there have been times I'd have liked having it. Edit: can't say I find the (longish) list The following files would have been hidden from the build system by trace mode if they existed: very helpful ...

I don't think you tested everything. Including old OS X versions, new ones, passing combinations of long, short options with and without parameters and invalid options.

No. I checked that the things I was looking for in the app worked. Truth be told, I half expect certain things not to work (without patching the code) given the nature of the beast and I hadn't even noticed that there was an option to use an included getopt version. Would you have raised an issue about getopt if that Makefile option had not been there?

Okay, they are shipping NetBSD's getopt. That's probably compatible enough.

Also check the rather basic argument list the app supports: nothing fancy that is likely to hinge on an exotic (or Swiss Army knife) version of getopt ..

Also move it after any modeline.

But then port lint won't be happy ;)

I don't really like putting the replacement stuff into configure. It's really patching, not running a configure script or program. But as it's more or less indeed "configuring" the package, I guess we can leave it as-is.

I know, but I've already seen configure scripts that do more or less the same thing ...

However, there are two problems with that: because you used use_configure no, there won't be an implicit universal variant. Please add one via variant universal {} after use_configure no.

I'm no longer using use_configure?! I checked: +universal is accepted and leads to a universal executable.

Secondly, you're way overcomplicating things.

Drop the variant_isset universal check.

reinplace -Os with ${configure.cppflags} ${configure.cflags} [get_canonical_archflags cc]

Hmm, so configure.universal_cppflags is never used (I noticed it was empty during my tests)?

and LDFLAGS := -lm with LDFLAGS := -lm ${configure.ldflags} [get_canonical_archflags ld]

Make sure to not drop -lm again!

Are you sure? I don't think that's required even on Linux anymore. It certainly didn't prove to be required (or else I'd have left it in). Edit: even when linking with -lm, there is no libm.dylib that shows up through otool -L.

[RJVB (René Bertin)]

The newly added dependencies

depends_build       port:perl5 \
                    port:p5-pathtools \
                    port:p5-scalar-list-utils

have an annoying side-effect:

>  port -v configure cpuid 
--->  p5-pathtools is replaced by p5.16-pathtools
--->  Deactivating p5-pathtools @3.470.0_1
--->  Cleaning p5-pathtools
--->  Removing work directory for p5-pathtools
--->  Computing dependencies for p5.16-pathtools.
--->  Cleaning p5.16-pathtools
--->  Removing work directory for p5.16-pathtools
--->  p5-scalar-list-utils is replaced by p5.16-scalar-list-utils
--->  Deactivating p5-scalar-list-utils @1.410.0_0
--->  Cleaning p5-scalar-list-utils
--->  Removing work directory for p5-scalar-list-utils
--->  Computing dependencies for p5.16-scalar-list-utils.
--->  Cleaning p5.16-scalar-list-utils
--->  Removing work directory for p5.16-scalar-list-utils
--->  Computing dependencies for cpuid...
--->  Dependencies to be installed: p5-pathtools p5-scalar-list-utils
--->  Activating p5-pathtools @3.470.0_1
[...]
x ./opt/local/share/doc/p5-scalar-list-utils/
x ./opt/local/share/doc/p5-scalar-list-utils/README
--->  Cleaning p5-scalar-list-utils
--->  Removing work directory for p5-scalar-list-utils
--->  Fetching distfiles for cpuid
--->  Verifying checksums for cpuid
--->  Checksumming cpuid-1.4.2.tar.gz
--->  Extracting cpuid
--->  Extracting cpuid-1.4.2.tar.gz
--->  Applying patches to cpuid
--->  Applying patch-makefile.diff
patching file GNUmakefile
--->  Configuring cpuid
--->  Patching GNUmakefile: s|-Os|-I/opt/local/include -Os -arch x86_64|g
--->  Patching GNUmakefile: s|LDFLAGS := -lm|LDFLAGS := -lm -L/opt/local/lib -Wl,-headerpad_max_install_names -arch x86_64|g
--->  Patching GNUmakefile: s|@@PREFIX@@|/opt/local|g

> port -v clean cpuid
--->  Cleaning cpuid
--->  Removing work directory for cpuid

> port -v configure cpuid
--->  p5-pathtools is replaced by p5.16-pathtools
--->  Deactivating p5-pathtools @3.470.0_1
--->  Cleaning p5-pathtools
--->  Removing work directory for p5-pathtools
--->  Computing dependencies for p5.16-pathtools.
--->  Cleaning p5.16-pathtools
--->  Removing work directory for p5.16-pathtools
--->  p5-scalar-list-utils is replaced by p5.16-scalar-list-utils
--->  Deactivating p5-scalar-list-utils @1.410.0_0
--->  Cleaning p5-scalar-list-utils
--->  Removing work directory for p5-scalar-list-utils
--->  Computing dependencies for p5.16-scalar-list-utils.
--->  Cleaning p5.16-scalar-list-utils
--->  Removing work directory for p5.16-scalar-list-utils
--->  Computing dependencies for cpuid...
--->  Dependencies to be installed: p5-pathtools p5-scalar-list-utils
--->  Activating p5-pathtools @3.470.0_1
x ./
x ./+COMMENT
[...]
x ./opt/local/share/doc/p5-scalar-list-utils/
x ./opt/local/share/doc/p5-scalar-list-utils/README
--->  Cleaning p5-scalar-list-utils
--->  Removing work directory for p5-scalar-list-utils
--->  Fetching distfiles for cpuid
--->  Verifying checksums for cpuid
--->  Checksumming cpuid-1.4.2.tar.gz
--->  Extracting cpuid
--->  Extracting cpuid-1.4.2.tar.gz
--->  Applying patches to cpuid
--->  Applying patch-makefile.diff
patching file GNUmakefile
--->  Configuring cpuid
--->  Patching GNUmakefile: s|-Os|-I/opt/local/include -Os -arch x86_64|g
--->  Patching GNUmakefile: s|LDFLAGS := -lm|LDFLAGS := -lm -L/opt/local/lib -Wl,-headerpad_max_install_names -arch x86_64|g
--->  Patching GNUmakefile: s|@@PREFIX@@|/opt/local|g

Why are p5-pathtools and p5-scalar-list-utils (re)installed over and over again? It's just a visual annoyance in this case, but imagine if this were to happen with a big port that cannot be distributed in binary form ...

I don't think there's anything wrong with my install, at least not with the p5 ports in question:

> port installed "*pathtools*" "*scalar-list*"
The following ports are currently installed:
  p5-pathtools @3.470.0_1 (active)
  p5-scalar-list-utils @1.410.0_0 (active)
  p5.16-pathtools @3.470.0_1 (active)
  p5.16-scalar-list-utils @1.410.0_0 (active)

[larryv (Lawrence Velázquez)]

I think this is a bug with the perl5 portgroup. Please open a new ticket for it.

In any case, you should not be depending on perl5 and p5-* ports. You can’t be sure that perl5 uses the Perl for which the module ports are installed. Use a specific version.

[RJVB (René Bertin)]

It would really annoy me to have to depend on a specific perl version... probably ought to use path style dependencies.

Anyway, I realise that the perl dependency should be automatic through the p5* dependencies.

[kurthindenburg (Kurt Hindenburg)]

Cc Me!

[kurthindenburg (Kurt Hindenburg)]

According to lint and github-1.0.tcl, the only valid github.tarball_from are releases, downloads and tags.

[RJVB (René Bertin)]

I submitted a (very simple) patch for the github portgroup (probably before submitting this port), but apparently wasn't the first. You'll find the modified github-1.0.tcl in my github repository.

[kurthindenburg (Kurt Hindenburg)]

OK,Ryan explained in another email that github archive isn't needed. It works with removing it - the checksums are different as he explains in 40518. I'll commit this unless someone has other issues.

[RJVB (René Bertin)]

I see an explanation about a change that would require every port using the group to be changed. I don't see how that would be necessary by just adding an option to tarball_from, and I must have missed the explanation how to get the tarball from an inexisting URL amidst the other clutter.

I tried that, but didn't succeed. EDIT: seems the tarball_from switch is optional and this works without using it at all?

[kurthindenburg (Kurt Hindenburg)]

yes, by default it works w/o the tarball_from

r137245