Page MenuHomeFreeBSD

Allow category_port_VARS= in make.conf for arbitrary per-port overrides.
ClosedPublic

Authored by andrew_tao173.riddles.org.uk on Apr 7 2020, 6:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sep 22 2024, 7:55 AM
Unknown Object (File)
Sep 14 2024, 7:05 PM
Unknown Object (File)
Sep 12 2024, 7:17 PM
Unknown Object (File)
Sep 12 2024, 7:17 PM
Unknown Object (File)
Sep 12 2024, 7:17 PM
Unknown Object (File)
Sep 12 2024, 7:17 PM
Unknown Object (File)
Sep 12 2024, 7:17 PM
Unknown Object (File)
Sep 12 2024, 7:13 PM

Details

Summary

(Per a request from koobs on irc)

Currently the only way to specify make.conf variables (other than port options which have their own mechanism) in a per-port fashion is to use conditionals on .CURDIR, which is fragile since it tends to involve assumptions about where the ports tree is mounted.

Instead, allow category_portname_VARS= to be set in make.conf to provide arbitrary assignments when building a specified port. For example one might use

devel_llvm10_VARS= MAKE_JOBS_NUMBER=2

or

converters_lua-iconv_VARS= TRYBROKEN=yes

This is intended to be consistent with the existing category_portname_SET= variables for port options, and uses the same syntax for values as option_VARS= in port makefiles, but without the case-folding behavior since that makes it impossible to handle mixed-case or lowercase variable names.

VAR+=val can be used to append to a variable, and VAR@ to unset one entirely.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

Background on request/motivation:

Was looking for a way to enable MAKE_JOBS globally in poudriere, but disable MAKE_JOBS for specific ports (like llvm, ghc, etc)

Poudriere provides a global make jobs toggle, and a per-port specific 'opt-in' variable ALLOW_MAKE_JOBS_PACKAGES. It doesn't provide the converse to 'opt-out' of make jobs on a per port basis

Adding this to the ports framework, as an extension of existing category_portname_{UN}SET functionality for OPTIONS, would as a bonus allow poudriere to be simplified too

Thanks for this @andrew_tao173.riddles.org.uk

Only a minor review comment to add basic inline feature description/usage comment

Will test this locally

Mk/bsd.port.mk
1310

Add brief comment block for feature description and usage/syntax line (in the absence of additional documentation added elsewhere). Something like:

# Per-port variable setting, like category_portname_{UN}SET from bsd.options.mk
# usage: /etc/make.conf: category_portname_VARS=<variable>=<value>

What difference is there from having either a Makefile.local with whatever you need in the port's directory, or using what everyone has been using for decades:

.if ${.CURDIR:M*/lang/rust*}
DISABLE_MAKE_JOBS=yes
.endif
Mk/bsd.port.mk
1307–1309

This is a very bad idea.

The default version of something is global and unique, it absolutely MUIST NOT be changed.

In D24324#543789, @mat wrote:

What difference is there from having either a Makefile.local with whatever you need in the port's directory, or using what everyone has been using for decades:

.if ${.CURDIR:M*/lang/rust*}
DISABLE_MAKE_JOBS=yes
.endif

Makefile.local means putting a file in the tree, which is probably not what you want if the same ports tree is being used for several package sets with different options.

The .if ${.CURDIR...} thing is error-prone (if you make it too general, it matches outside the ports tree; too specific, and it'll fail to match depending on where the ports tree got mounted).

I really like the idea of being able to simply and cleanly set vars for specific ports from /etc/make.conf. I think the choice of variable naming is exactly right here, and the implementation is clean and simple.

However, I am 100% with @mat here that bsd.default-versions.mk is the wrong place for this. The idea is that the things defined in there choose one from a list of conflicting options. If every port is built with ssl=openssl and one port is overridden with ssl=libressl, then it will be impossible to install that port without uninstalling everything else. I can't think of a scenario where any default version should be overridden.

Can you implement this somewhere else, so that default versions are preserved?

I really like the idea of being able to simply and cleanly set vars for specific ports from /etc/make.conf. I think the choice of variable naming is exactly right here, and the implementation is clean and simple.

However, I am 100% with @mat here that bsd.default-versions.mk is the wrong place for this. The idea is that the things defined in there choose one from a list of conflicting options. If every port is built with ssl=openssl and one port is overridden with ssl=libressl, then it will be impossible to install that port without uninstalling everything else. I can't think of a scenario where any default version should be overridden.

Can you implement this somewhere else, so that default versions are preserved?

I put it there specifically because, prior to getting the lua flavors work in, I was having to override versions all the time to deal with conflicts over what lua version was needed for a given dependency. (e.g. Port X needs port Y built for lua52, even when the default is 5.3, or I could keep the default on 5.2 and then have to override it again via DEFAULT_VERSIONS for port Z required for my own development work under 5.3).

That specific issue may be solved now, but it's an example of why "I can't think of a scenario..." cuts no ice with me.

Incidentally, in case it's not obvious, if my addition is moved to before the inclusion of bsd.default-versions.mk then it remains entirely possible to do

category_port_VARS= default_versions+=foo=123

which would have the same effect as category_port_VARS= foo_version=123 with the current placement.

Maybe that's better style, but it's not any different functionally.

Move to an earlier place in processing (in fact the earliest possible place, just after PKGORIGIN is set).

Change the comments a bit to include usage and example.

Discussion point: this implementation converts the variable name to upper case for consistency with the way that port option_VARS works. But there are some cases where one might want to set a mixed-case name (such as CFLAGS.arch), so maybe it shouldn't do that and just use the case as written?

Also, it looks like a way to undefine a variable would also be useful, since sometimes setting an empty value isn't enough.

Discussion point: this implementation converts the variable name to upper case for consistency with the way that port option_VARS works. But there are some cases where one might want to set a mixed-case name (such as CFLAGS.arch), so maybe it shouldn't do that and just use the case as written?

I'd say there is no point in supporting all-lowercase variable names (e.g., disable_make_jobs) instead of just the actual names (e.g., DISABLE_MAKE_JOBS). I must say that I'm a bit confused why option_VARS is supposedly supporting such a syntactic sugar.

In D24324#630061, @0mp wrote:

Discussion point: this implementation converts the variable name to upper case for consistency with the way that port option_VARS works. But there are some cases where one might want to set a mixed-case name (such as CFLAGS.arch), so maybe it shouldn't do that and just use the case as written?

I'd say there is no point in supporting all-lowercase variable names (e.g., disable_make_jobs) instead of just the actual names (e.g., DISABLE_MAKE_JOBS). I must say that I'm a bit confused why option_VARS is supposedly supporting such a syntactic sugar.

I'm with both of you on that. It's pure sugar, and since it was created we've started using mixed-case variable names more often.

Don't fold case of variable names. Allow using var@ to unset variables, since some places distinguish between empty and undefined values.

This is actually the version I have been using locally for a while now.

This would be a great thing to have. Can someone weigh in on the implementation?

I like the idea we should just make sure imho we have enough safety belt to prevent as much as possible self shooting

Mk/bsd.port.mk
1287

here it should probably be something like ?= to avoid overwritins vars that are already defined by the ports tree at this stage

Mk/bsd.port.mk
1287

No, overriding variables is exactly the point, so = is correct.

Mk/bsd.port.mk
1287

This makes it dangerous! and open the gates to very complicated cases to diagnose.
the ?= make your MAKE_JOBS_NUMBER example functional, aka present what is meant to be preset but per port.
otherwise it will open a very different can of worm which I am really not sure it is worth opening (I can be convinced otherwise)

The obvious example is doing this in make.conf:

# suppose I have 4 cpus but don't want to tie them all up
MAKE_JOBS_NUMBER=3
# but some ports would use too much ram:
devel_llvm13_VARS= MAKE_JOBS_NUMBER=2

Secondly, using ?= is trivially defeated by setting cat_port_VARS= BLAH@ BLAH=whatever to unset the variable first and then reset it.

Mk/bsd.port.mk
1287

At the end of the day, is this any more dangerous than Makefile.local? Makefile.local can cause strange failures, and while they do show up occasionally in bug reports and on lists, I'm not under the impression that it's a prohibitive problem.

Plus, when people do have strange failures, one of the first pieces of advice already is to look at /etc/make.conf. So really, if people do shoot themselves in foot, our docs already tell them where to look. Plus, the majority of make.conf errors come from defining something that affects more ports than intended, which is what this patch tries to mitigate.

I tend toward being idealistic, so take this with a grain of salt, but I like that FreeBSD gives users substantial control over their systems. It takes a significant level of port Makefile comprehension to overwrite something, so I feel like we're exposing the greatest risk to people who are most likely to understand what they're doing.

The risk comes for people who are savvy enough figure out how to overwrite a variable defined in a Makefile, but who can't figure out how to undo it (i.e. delete a line in a file that they had to manually create).

My take (and to reiterate, my perspective on these things is pretty non-technical) is that if too many people shoot themselves in the foot, we can always narrow the scope. I like it when we err on the side of trusting our user base.

This revision is now accepted and ready to land.Oct 10 2021, 5:55 PM

This would be really useful in my goal to -O2 or -O3 compile (via poudriere) specific, performance-sensitive packages for a CPUTYPE that is different from the poudriere host machine. I can't set CPUTYPE for all packages, as the build fails right away when pkg won't run on the host machine; this would also be a problem with innumerable other build dependencies. I would, however, like to set CPUTYPE for postgresql13, for example, which AFAIK is never executed as a build dependency. Another possible benefit of an automated, optimized build is openjdk*.

so... two years after everyone seems to have accepted it, maybe someone would commit it?

so... two years after everyone seems to have accepted it, maybe someone would commit it?

I'm not on portmgr anymore, so now I'm just another committer with a +1 :-)

@rene Could you perhaps see if this could be committed?

I think we should only commit with according additions to CHANGES and the porter's handbook, to avoid astonishment.

Also date-wise, this seems a bit close to 2023Q3. If it were my call alone, I'd fork 2023Q3 first on July 1st and only after that commit stuff that might break things.

The patch literally does nothing unless the variables it checks for are defined. Who is going to be surprised?

As for documentation, it doesn't belong in the porter's handbook any more than cat_port_SET=... etc. do; the place it should go is probably in the ports(7) manpage. You'll notice, however, that cat_port_SET are not documented their either (nor were they ever mentioned in CHANGES); the only place you'll find any mention of those is in Mk/bsd.options.mk.

The time to object about the timing was two years ago, or three weeks ago.

I plan to commit this one with my next batch modulo indentation changes as required by Tools/scripts/indent_make_if.pl.