Paths

Table of Contentst

Differential D47850

Mk/bsd.port.mk: stop ports from finding the ports tree's git repository
ClosedPublic
Actions

Authored by fuz on Nov 30 2024, 1:39 PM.

Details

Reviewers

None

Group Reviewers

portmgr

Commits

R11:deb79782a9b8: Mk/bsd.port.mk: stop ports from finding the ports tree's git repository

Summary

When building from source, the ports tree is by default a parent
directory of WRKDIR. When ports try to use git to find the git commit
they are built from, they'll some times find the port's git repository
and produce bogus metadata. Set GIT_CEILING_DIRS in WRK_ENV to ensure
that git does not escape WRKDIR trying to look for a git repository
where we have extracted the distfile.

This doesn't affect Poudriere builds as for these, WRKDIR is by default
not a subdirectory of the ports tree, and neither is git installed in
builder jails unless explicitly needed. As a result, bugs of this kind
tend to go unnoticed by maintainers, but do affect source builders.

Diff Detail

Repository

R11 FreeBSD ports repository

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 60902
Build 57786: arc lint + arc unit

Event Timeline

fuz requested review of this revision.Nov 30 2024, 1:39 PM

fuz created this revision.

Harbormaster completed remote builds in B60888: Diff 147183.Nov 30 2024, 1:39 PM

fuz edited the summary of this revision. (Show Details)Nov 30 2024, 1:44 PM

This looks like a good idea, but could you please add the new environment variable to where WRK_ENV is first appended to, about 100 lines before.

kenrap_kennethraplee.com added a subscriber: kenrap_kennethraplee.com.Nov 30 2024, 7:47 PM

Mk/bsd.port.mk: move GIT_CEILING_DIRECTORIES to initial WRK_ENV block

Harbormaster completed remote builds in B60902: Diff 147323.Dec 1 2024, 5:07 PM

mat added inline comments.Dec 2 2024, 6:37 AM

Mk/bsd.port.mk
1623	Why `:H` here ? `${WRKDIR}` should be enough.

fuz marked an inline comment as done.Dec 2 2024, 7:19 AM

fuz added inline comments.

Mk/bsd.port.mk
1623	`GIT_CEILING_DIRECTORIES` is a list of directories git will not change into when looking for git repositories. It is conceivable that a port would use `WRKDIR` for a git repository (even though that sounds a bit weird), so the first directory that is forbidden is the parent directory of `WKRDIR`.

And when that is changed, please ask for an exp-run to see if things break.

Mk/bsd.port.mk
1623	Yeah, no, ports are always extracted in a subdirectory of WRKDIR because other things get stored there. If there are ports that break, they are probably doing something stupid and should be fixed. Please change it to `${WRKDIR}`.

arrowd added inline comments.Dec 2 2024, 7:09 PM

Mk/bsd.port.mk
1623	Yeah, no, ports are always extracted in a subdirectory of WRKDIR because other things get stored there. If there are ports that break, they are probably doing something stupid and should be fixed. It is not uncommon that a distfile does not contain a single subdir. We even have https://cgit.freebsd.org/ports/tree/Mk/bsd.port.mk#n460 for such cases. I think `${WRKDIR:H}` is a better fit here.

fuz marked 2 inline comments as done.Dec 2 2024, 11:24 PM

fuz added inline comments.

Mk/bsd.port.mk
1623	Using `${WRKDIR}` is insufficient. If that was used, a git command executed in `WRKDIR` (not unlikely!) would traverse upwards to find the ports tree, as `GIT_CEILING_DIRECTORIES` is only queried when changing directories, but not when the working directory is checked for a `.git` directory. If you also want to ignore git repositories in `WRKDIR`, I can add both `WRKDIR` and `WRKDIR:H` to `GIT_CEILING_DIRECTORIES`.

No git command should be run in WRKDIR, ever. If a port does this, it should be fixed.

You somewhat assume that I don't understand what you mean, I do, I rewrote enough of the framework over the years to have an intimate knowledge of how it works, and how ports should work.
If a port gets broken because of this change, which is unlikely, that particular port should be fixed.

If you are unwilling to go through the hoops of working with portmgr when proposing a patch for the framework, please say so. Your change has value, and it should be included, but it should be included properly. If you do not want to change it as is required, I can take it over from here and finish the task. (Changing to ${WRKDIR} and going through with the exp-run.)

Mk/bsd.port.mk
1623	When a port's distfile does not create a subdirectory, it has to use NO_WRKSUBDIR, which magically creates a subdirectory for it, so WRKDIR is enough. The comment in there is somewhat wrong. As I said, a port that does not do this it is broken and should be fixed.
1623	Can you please do as I ask ?

mat added inline comments.Dec 3 2024, 6:57 AM

Mk/bsd.port.mk
1623	Ah, no, the comment is exactly right, I read it too fast. When you set NO_WRKSUBDIR, the framework will magically create a WRKSRC based on the name of the package, and will extract inside WRKSRC, not inside WRKDIR, so a subdirectory is always present.

Hi @mat, I once again appreciate your time and reviews. It is my intention to justify the design choices I made and why I believe them to be important. Of course it's your final call to decide how things should be done and I respect that.

Mk/bsd.port.mk
1623	I just don't get why you want me to add an unnecessary hole to this failsafe. Any port that tries to run `git` to detect its version is already broken and clearly those do not get fixed on a regular basis as I keep seeing ports that do so. So I don't see how adding a hole to the failsafe is going to improve things. It just doesn't make sense to me. But if that is your wish, I will make the failsafe defective as you indicate.

Mk/bsd.port.mk Use WRKDIR instead of WRKDIR:H for GIT_CEILING_DIRECTORIES

Harbormaster completed remote builds in B60925: Diff 147397.Dec 3 2024, 12:58 PM

Exp-run requested in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=283098.

With the exp-run being successful, do I have permission to commit?

Yes, please, go ahead.

A lot of Python ports executes git but I haven't looked into why I do agree though that this is just a "cosmetic" fix rather than fixing the ports in question.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 26 2024, 10:47 AM

Closed by commit R11:deb79782a9b8: Mk/bsd.port.mk: stop ports from finding the ports tree's git repository (authored by fuz). · Explain Why

This revision was automatically updated to reflect the committed changes.

fuz added a commit: R11:deb79782a9b8: Mk/bsd.port.mk: stop ports from finding the ports tree's git repository.