Page MenuHomeFreeBSD

The practice of creating symbolic links is somewhat fragile. Make hard copies instead always.
ClosedPublic

Authored by imp on Aug 5 2020, 9:09 PM.

Details

Summary

There's too many times that we can't run the new binaries with old
libraries. Making the links when things are known to be 'safe' is a
nice optimization, but a copy of all the binaries is only 30MB, so
saving the copies at the cost of increased support when new symbols
are added and used as part of the bootstrap seems to be unwise.

Test Plan

Note: this likely requires some comments to be adjusted.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

imp requested review of this revision.Aug 5 2020, 9:09 PM
imp added reviewers: bdrewery, arichardson, kevans.
rpokala added inline comments.
Makefile.inc1
2289 ↗(On Diff #75452)

Do we want to delete the old version before checking for the existence of the new? Or would it be better to check for the existence of the new, and then remove the old?

For that matter, since you're using cp -f, the deletion is already part of that: from cp(1):

-f    For each existing destination pathname, remove it and create a new
      file, without prompting for confirmation regardless of its
      permissions.  (The -f option overrides any previous -i or -n
      options.)
tools/build/Makefile
122 ↗(On Diff #75452)

You introduced a @ in front of source_path; intentional, or accidental?

127 ↗(On Diff #75452)

Same comment as above; cp -f means the rm is redundant.

136 ↗(On Diff #75452)

And here.

140 ↗(On Diff #75452)

And here too.

Makefile.inc1
2289 ↗(On Diff #75452)

Yes. I do. And I have to do it first otherwise, the which will pick up the current copy here, defeating the purpose of the which...

tools/build/Makefile
122 ↗(On Diff #75452)

Yes. Intentional. @ means to not print this command in make and the if had it before...

127 ↗(On Diff #75452)

Here and below I got an error that the two files were identical w/o the -f flag.

tools/build/Makefile
127 ↗(On Diff #75452)

Here and below I got an error that the two files were identical w/o the -f flag.

That's fine, include the -f, but in that case, the rm -f shouldn't be necessary.

Makefile.inc1
2289 ↗(On Diff #75452)

Also, checking the source, the check for identical files occurs before the use fflag to do the copy anyway.

tools/build/Makefile
127 ↗(On Diff #75452)

If I don't remove, then I get the error. The -f flag to rm is needed in case the file isn't here yet. the -f flag to cp may not truly be needed...

Copying instead of creating links *should* be fine. However, I vaguely seem to recall this previously caused issues for bootstrapping macOS since one tool wanted to find something relative to realpath argv[0]. Will check if that's still the case tomorrow morning UK time.

Copying instead of creating links *should* be fine. However, I vaguely seem to recall this previously caused issues for bootstrapping macOS since one tool wanted to find something relative to realpath argv[0]. Will check if that's still the case tomorrow morning UK time.

Sure thing. I included you because I had hoped to get this perspective. On Mac/Linux, only the incompatible tools need to be bootstrapped, and they will use $HOST libraries always, no?

In D25967#575682, @imp wrote:

Copying instead of creating links *should* be fine. However, I vaguely seem to recall this previously caused issues for bootstrapping macOS since one tool wanted to find something relative to realpath argv[0]. Will check if that's still the case tomorrow morning UK time.

Sure thing. I included you because I had hoped to get this perspective. On Mac/Linux, only the incompatible tools need to be bootstrapped, and they will use $HOST libraries always, no?

Yes, and they default to BOOTSTRAP_ALL_TOOLS.

In D25967#575682, @imp wrote:

Copying instead of creating links *should* be fine. However, I vaguely seem to recall this previously caused issues for bootstrapping macOS since one tool wanted to find something relative to realpath argv[0]. Will check if that's still the case tomorrow morning UK time.

Sure thing. I included you because I had hoped to get this perspective. On Mac/Linux, only the incompatible tools need to be bootstrapped, and they will use $HOST libraries always, no?

Yes, and they default to BOOTSTRAP_ALL_TOOLS.

So they never use the host native binaries once bootsrapped?

In D25967#575752, @imp wrote:
In D25967#575682, @imp wrote:

Copying instead of creating links *should* be fine. However, I vaguely seem to recall this previously caused issues for bootstrapping macOS since one tool wanted to find something relative to realpath argv[0]. Will check if that's still the case tomorrow morning UK time.

Sure thing. I included you because I had hoped to get this perspective. On Mac/Linux, only the incompatible tools need to be bootstrapped, and they will use $HOST libraries always, no?

Yes, and they default to BOOTSTRAP_ALL_TOOLS.

So they never use the host native binaries once bootsrapped?

Well the ALL in BOOTSTRAP_ALL_TOOLS is not quite true. Certain basic tools such as cp, mv, etc. are never bootstrapped.

Additionally, crossbuilding enforces -DNO_ROOT so the LD_LIBRARY_PATH issue during install doesn't show up.

I can confirm that cross-building from macOS still works with this patch. I have a slight preference for keeping symlinks in the ${.MAKE.OS} != "FreeBSD" / -DNO_ROOT case but keeping stuff consistent is more important.

This revision is now accepted and ready to land.Aug 6 2020, 10:43 AM