Opened 22 months ago
Closed 19 months ago
#66751 closed defect (fixed)
snac @2.18: use the right compiler and flags
Reported by: | ryandesign (Ryan Carsten Schmidt) | Owned by: | artkiver (グレェ) |
---|---|---|---|
Priority: | Normal | Milestone: | |
Component: | ports | Version: | 2.8.0 |
Keywords: | haspatch | Cc: | |
Port: | snac |
Description
snac isn't UsingTheRightCompiler nor flags:
cc -g -Wall -D st_mtim=st_mtimespec -I/opt/local/include -c main.c cc -g -Wall -D st_mtim=st_mtimespec -I/opt/local/include -c data.c cc -g -Wall -D st_mtim=st_mtimespec -I/opt/local/include -c http.c cc -g -Wall -D st_mtim=st_mtimespec -I/opt/local/include -c httpd.c cc -g -Wall -D st_mtim=st_mtimespec -I/opt/local/include -c webfinger.c cc -g -Wall -D st_mtim=st_mtimespec -I/opt/local/include -c activitypub.c cc -g -Wall -D st_mtim=st_mtimespec -I/opt/local/include -c html.c
The port is manually setting use_configure no
and not doing any of the manual work needed to support this properly.
The makefile portgroup could help.
Attachments (1)
Change History (8)
comment:1 Changed 22 months ago by artkiver (グレェ)
comment:2 follow-up: 5 Changed 22 months ago by artkiver (グレェ)
So, snac 2.19 was released, which reminded me to look into this in more detail.
https://guide.macports.org/chunked/reference.portgroup.html is a bit sparse as far as information on the makefile PortGroup.
https://github.com/macports/macports-ports/blob/master/_resources/port1.0/group/makefile-1.0.tcl
Is I guess a useful reference insomuch as it is the TCL being invoked, though I am not sure how well I am parsing it as a human.
Regardless, doing some minor edits to the Portfile
e.g. PortGroup makefile 1.0
Still seems to install OK after I do a port -v clean.
In reading, I thought about adding:
build.args-append CFLAGS=-g -Wall -D st_mtim=st_mtimespec
And it doesn't seem to cause harm?
I even explored removing the
patchfiles Makefile.patch post-patch {reinplace "s|/usr/local|${prefix}|g" ${worksrcpath}/Makefile}
As well as attempting to add some other things to build.args-append
Such as:
PREFIX=/opt/local and PREFIX_MAN=/opt/local/share/man
But I may have been a bit too ambitious with some of that and seemed to get some errors back when running lint --nitpick for example.
So, at the moment, I have a rather more conservative Portfile with some changes, which seems to function OK; but I am not sure if it addresses all the issues raised here.
I'm attaching it for reference though. Doubtlessly, it can be improved upon somewhat, but I figure best to update the Trac ticket for the time being as no one has replied on IRC to my inquiry yet.
Changed 22 months ago by artkiver (グレェ)
Attachment: | Portfile.2.19.makefileportgroupandsuch added |
---|
Portfile for net/snac which adds the makefile PortGroup, updates the version and checksums for 2.19, updates the long_description and tests OK locally.
comment:3 Changed 22 months ago by artkiver (グレェ)
Well, what "worksforme" on my local system apparently made the build bots unhappy when I submitted a PR:
e.g.
https://github.com/macports/macports-ports/actions/runs/4021961917/jobs/6911272754
https://github.com/macports/macports-ports/actions/runs/4021961917/jobs/6911272700
I can, presumably, revert to an even more conservative Portfile version bump and I am guessing it would pass the CI checks OK, but probably not adequately address the concerns raised here.
comment:4 Changed 21 months ago by artkiver (グレェ)
A more conservative PR was submitted here: https://github.com/macports/macports-ports/pull/17799
To update snac to 2.23 to be in alignment with the upstream project.
Thankfully that PR passed the CI build bot checks at least unlike my previous documented attempt, but omits the recommendations and other efforts described in this ticket, even if it does at least address the concerns raised in this one:
So, for the time being I am updating this ticket and leaving it open as a reminder that I have additional effort to put in so as to refactor the Portfile to utilize the makefile PortGroup.
comment:5 follow-up: 6 Changed 19 months ago by ryandesign (Ryan Carsten Schmidt)
Keywords: | haspatch added |
---|
Replying to artkiver:
https://guide.macports.org/chunked/reference.portgroup.html is a bit sparse as far as information on the makefile PortGroup.
https://github.com/macports/macports-ports/blob/master/_resources/port1.0/group/makefile-1.0.tcl
Is I guess a useful reference insomuch as it is the TCL being invoked, though I am not sure how well I am parsing it as a human.
Yes, the portfile code, and the comments at the top of that file, are the documentation at present.
Regardless, doing some minor edits to the Portfile
e.g. PortGroup makefile 1.0
Still seems to install OK after I do a port -v clean.
Yes, but the entire purpose of the makefile portgroup is to assist you with projects that use custom Makefiles. As such, there is no one-size-fits-all solution and it is a virtual certainty that you will need to analyze the project's Makefile, and the settings available in the makefile portgroup, and adjust those settings to match what's needed for this Makefile and/or patch the Makefile to make it accept the level of customization we require.
In reading, I thought about adding:
build.args-append CFLAGS=And it doesn't seem to cause harm?
The correct way to augment the CFLAGS
of a MacPorts port would be to -append
to the configure.cflags
variable:
configure.cflags-append -g -Wall -D st_mtim=st_mtimespec
The Makefile already puts -g -Wall
into the CFLAGS
but only if they weren't already set (and once we use the makefile portgroup, they will already be set). I think it's advisable to patch the Makefile so that it appends these flags to any existing value already present for CFLAGS
.
I even explored removing the
patchfiles Makefile.patch post-patch {reinplace "s|/usr/local|${prefix}|g" ${worksrcpath}/Makefile}
The makefile portgroup takes care of setting the PREFIX
variable so if that were the only place where /usr/local occurred then you could remove the post-patch reinplace. But it isn't: the Makefile contains many other hardcoded references to /usr/local which also need to be replaced. I'll try to update the patch to fix these so the reinplace is no longer needed. This problem should be reported to the developers so that they can incorporate this patch or fix it another way.
The Makefile.patch shouldn't be removed because it currently does two things which are still needed (and will need to be expanded to fix additional problems):
- It defines
st_mtim=st_mtimespec
which still needs to be done either there or by appending to theCFLAGS
in the Portfile; the CI build logs you provided showed the build failed because this was not done. Since this should only be done on specific operating systems like Darwin, it's probably better to set it in aplatform
block in the Portfile. This problem should be reported to the developers so that they can modify their build system to set this on Darwin so we don't have to do it ourselves. - It teaches the Makefile how to support
DESTDIR
which is still desirable and which should be submitted to the developers of this software so they can incorporate it (although the makefile portgroup can be used whether or not a Makefile supportsDESTDIR
; just setmakefile.has_destdir
correctly).
As well as attempting to add some other things to build.args-append
Such as:
PREFIX=/opt/local and PREFIX_MAN=/opt/local/share/man
The makefile portgroup sets PREFIX
for you so you don't need to do it here again however the makefile portgroup sets PREFIX
as an environment variable. Any variable set unconditionally in the Makefile will override an environment variable, and this Makefile does unconditionally set PREFIX
. The solution is to either modify the Makefile so that it sets PREFIX
only if it has not already been set (PREFIX?=/usr/local
) or to use the makefile portgroup's option makefile.override-append PREFIX
to tell it to set a build argument instead of an environment variable.
The Makefile defaults PREFIX_MAN
to $(PREFIX)/man
which is not the correct value today, so you're right that you'll need to set it to ${prefix}/share/man, either by specifying PREFIX_MAN
in build.args
or in the patchfile. In fact some work had already been done in the patchfile to correct the manpage installation directory but it was done wrong.
All of this should be fixed in this PR: https://github.com/macports/macports-ports/pull/18212
comment:6 Changed 19 months ago by artkiver (グレェ)
Awesome, thank you for the informative and detailed reply! I have only skimmed it for the time being and am about to embark on a road trip, but I wanted to at least acknowledge the effort you put into this now before I am mostly away from a keyboard. I look forward to revisiting this as I am able!
Replying to ryandesign:
Replying to artkiver:
https://guide.macports.org/chunked/reference.portgroup.html is a bit sparse as far as information on the makefile PortGroup.
https://github.com/macports/macports-ports/blob/master/_resources/port1.0/group/makefile-1.0.tcl
Is I guess a useful reference insomuch as it is the TCL being invoked, though I am not sure how well I am parsing it as a human.
Yes, the portfile code, and the comments at the top of that file, are the documentation at present.
Regardless, doing some minor edits to the Portfile
e.g. PortGroup makefile 1.0
Still seems to install OK after I do a port -v clean.
Yes, but the entire purpose of the makefile portgroup is to assist you with projects that use custom Makefiles. As such, there is no one-size-fits-all solution and it is a virtual certainty that you will need to analyze the project's Makefile, and the settings available in the makefile portgroup, and adjust those settings to match what's needed for this Makefile and/or patch the Makefile to make it accept the level of customization we require.
In reading, I thought about adding:
build.args-append CFLAGS=And it doesn't seem to cause harm?
The correct way to augment the
CFLAGS
of a MacPorts port would be to-append
to theconfigure.cflags
variable:configure.cflags-append -g -Wall -D st_mtim=st_mtimespecThe Makefile already puts
-g -Wall
into theCFLAGS
but only if they weren't already set (and once we use the makefile portgroup, they will already be set). I think it's advisable to patch the Makefile so that it appends these flags to any existing value already present forCFLAGS
.I even explored removing the
patchfiles Makefile.patch post-patch {reinplace "s|/usr/local|${prefix}|g" ${worksrcpath}/Makefile}The makefile portgroup takes care of setting the
PREFIX
variable so if that were the only place where /usr/local occurred then you could remove the post-patch reinplace. But it isn't: the Makefile contains many other hardcoded references to /usr/local which also need to be replaced. I'll try to update the patch to fix these so the reinplace is no longer needed. This problem should be reported to the developers so that they can incorporate this patch or fix it another way.The Makefile.patch shouldn't be removed because it currently does two things which are still needed (and will need to be expanded to fix additional problems):
- It defines
st_mtim=st_mtimespec
which still needs to be done either there or by appending to theCFLAGS
in the Portfile; the CI build logs you provided showed the build failed because this was not done. Since this should only be done on specific operating systems like Darwin, it's probably better to set it in aplatform
block in the Portfile. This problem should be reported to the developers so that they can modify their build system to set this on Darwin so we don't have to do it ourselves.- It teaches the Makefile how to support
DESTDIR
which is still desirable and which should be submitted to the developers of this software so they can incorporate it (although the makefile portgroup can be used whether or not a Makefile supportsDESTDIR
; just setmakefile.has_destdir
correctly).As well as attempting to add some other things to build.args-append
Such as:
PREFIX=/opt/local and PREFIX_MAN=/opt/local/share/manThe makefile portgroup sets
PREFIX
for you so you don't need to do it here again however the makefile portgroup setsPREFIX
as an environment variable. Any variable set unconditionally in the Makefile will override an environment variable, and this Makefile does unconditionally setPREFIX
. The solution is to either modify the Makefile so that it setsPREFIX
only if it has not already been set (PREFIX?=/usr/local
) or to use the makefile portgroup's optionmakefile.override-append PREFIX
to tell it to set a build argument instead of an environment variable.The Makefile defaults
PREFIX_MAN
to$(PREFIX)/man
which is not the correct value today, so you're right that you'll need to set it to ${prefix}/share/man, either by specifyingPREFIX_MAN
inbuild.args
or in the patchfile. In fact some work had already been done in the patchfile to correct the manpage installation directory but it was done wrong.All of this should be fixed in this PR: https://github.com/macports/macports-ports/pull/18212
comment:7 Changed 19 months ago by ryandesign (Ryan Carsten Schmidt)
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Ah, thanks. I will look into this further. I figured there was probably a better way to do this earlier, but I admit I am not super up to speed on portgroups. I will do additional research and experimentation and hopefully get this smoothed out a bit!