Details

Reviewers

imp
emaste
sjg

Commits

rS325330: Enable AUTO_OBJ by default if the OBJDIR is writable, only for in-tree builds.

Summary

This can be disabled by putting WITHOUT_AUTO_OBJ=yes in /etc/src-env.conf, not
/etc/src.conf, or passing it in the environment.

The purpose of this rather than simply flipping the default of AUTO_OBJ to yes
is to avoid hassling users with auto.obj.mk failures if the wanted OBJDIR is
not writable. It will fallback to writing to the source directory like it does
today if MAKEOBJDIRPREFIX is not writable.

The act of enabling MK_AUTO_OBJ disables all 'make obj' treewalks since
previous work has made those not run if MK_AUTO_OBJ==yes in Makefile.inc1.

Relnotes: yes
Sponsored by: Dell EMC Isilon

Test Plan

/root/git/freebsd2/bin/sh % id
uid=1003(bryan) gid=1003(bryan) groups=1003(bryan),5(operator)
/root/git/freebsd2/bin/sh % mkdir -p /usr/obj/root/git/freebsd2/bin/sh
mkdir: /usr/obj/root/git/freebsd2: Permission denied

# This user has no write access to the default wanted OBJDIR
# so it defaults to the current directory as normal.

/root/git/freebsd2/bin/sh % make -V .OBJDIR
/root/git/freebsd2/bin/sh
% make
Warning: Object directory not changed from original /root/git/freebsd2/bin/sh
...

# Providing a MAKEOBJDIRPREFIX that the user can write to enables
# AUTO_OBJ automatically.

/root/git/freebsd2/bin/sh % rm -rf /tmp/root
/root/git/freebsd2/bin/sh % MAKEOBJDIRPREFIX=/tmp make -V .OBJDIR
[Creating objdir /tmp/root/git/freebsd2/amd64.amd64/bin/sh...]
/tmp/root/git/freebsd2/amd64.amd64/bin/sh
/root/git/freebsd2/bin/sh % rm -rf /tmp/root
/root/git/freebsd2/bin/sh % MAKEOBJDIRPREFIX=/tmp make
[Creating objdir /tmp/root/git/freebsd2/amd64.amd64/bin/sh...]
Building /tmp/root/git/freebsd2/amd64.amd64/bin/sh/builtins.c
...



# Without this change this is the result of setting AUTO_OBJ to
# default yes for a user:

/root/git/freebsd2/bin/sh % make
[Creating objdir /usr/obj/root/git/freebsd2/amd64.amd64/bin/sh...]
mkdir: /usr/obj/root/git/freebsd2: Permission denied
make: "/root/git/freebsd2/share/mk/auto.obj.mk" line 67: could not use /usr/obj/root/git/freebsd2/amd64.amd64/bin/sh: .OBJDIR=/root/git/freebsd2/bin/sh

universe/buildworld,native-xtools/xdev/installworld/installkernel all fine. Ran into unrelated issue fixed in rS325249 which made me think these were failing.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 12316
Build 12605: arc lint + arc unit

Event Timeline

bdrewery created this revision.Oct 31 2017, 12:43 AM

Harbormaster completed remote builds in B12313: Diff 34475.Oct 31 2017, 12:43 AM

bdrewery mentioned this in D12840: Add option UNIFIED_OBJDIR, on by default, which moves the default build OBJDIR..Oct 31 2017, 12:44 AM

bdrewery added a subscriber: • ian.Oct 31 2017, 12:48 AM

Export MK_AUTO_OBJ==yes finding to sub-makes. This was not being checked
very often in buildworld but exporting the positive finding will save more time.

Harbormaster completed remote builds in B12314: Diff 34476.Oct 31 2017, 12:51 AM

Uploaded the wrong commit, fix that.

Harbormaster completed remote builds in B12315: Diff 34477.Oct 31 2017, 12:52 AM

bdrewery added a parent revision: D12840: Add option UNIFIED_OBJDIR, on by default, which moves the default build OBJDIR..Oct 31 2017, 12:56 AM

Don't bother trying to enable MK_AUTO_OBJ if WITHOUT_AUTO_OBJ is set or
MK_AUTO_OBJ is in .MAKEOVERRIDES.

Harbormaster completed remote builds in B12316: Diff 34478.Oct 31 2017, 1:56 AM

Disable opportunistic AUTO_OBJ for showconfig/print-dir

Harbormaster completed remote builds in B12318: Diff 34480.Oct 31 2017, 2:33 AM

Fix 'make print-dir' mistake, add WITHOUT_AUTO_OBJ man file and force "on" for 'make showconfig'.
Also tested that makeman did the right thing and shows AUTO_OBJ as default enabled.

trap more signals for rmdir cleanup.
This should be final.

Harbormaster completed remote builds in B12322: Diff 34485.Oct 31 2017, 3:06 AM

bdrewery edited the test plan for this revision. (Show Details)Oct 31 2017, 8:48 PM

bdrewery edited the test plan for this revision. (Show Details)Oct 31 2017, 8:53 PM

bdrewery edited the test plan for this revision. (Show Details)Oct 31 2017, 9:55 PM

sjg added inline comments.Oct 31 2017, 11:46 PM

share/mk/src.sys.obj.mk
93	If this is not bound to `${.MAKE.LEVEL} == 0` you have race conditions to deal with below.
107	There's a race condition here you need to account for. Which is why `Mkdirs` in `auto.obj.mk` tries `mkdir -p` more than once. Between your `[ -d $${dir} ]` and `mkdir $${dir}` another instance of make may have made the same intermediate directory. `mkdir -p` deals with this case sanely now, but used to throw an error. The above has the same issue as early `mkdir -p`
123	This can fail - and should do so gracefully, in the case that another make process has created other subdirs.

bdrewery added inline comments.Nov 1 2017, 12:08 AM

share/mk/src.sys.obj.mk
93	Good point. It's intended to be. I'll add a `${.MAKE.LEVEL} == 0` check here. If it succeeds it does export `MK_AUTO_OBJ=yes` so it should cover all sub-makes.

bdrewery added inline comments.Nov 1 2017, 5:02 PM

share/mk/src.sys.obj.mk
107	I am speaking only of auto.obj.mk here... By the way, the FreeBSD `mkdir -p` does not have a problem with concurrent `mkdir -p <samepath>` running. If a component for `mkdir(2)` returns `EEXIST` it just ignores the error unless it is the last component, and then `build` returns `2` which `main` does nothing with. It looks like NetBSD's mkdir handles it similarly. Just thinking about this, doing `mkdir -p /usr/obj/foo` would hit the race you're worried about in auto.obj.mk since both of '/' and '/usr' will exist already. The nature of the -p flag makes this not a problem for auto.obj.mk. Perhaps there is an issue on NFS?

bdrewery retitled this revision from Enable AUTO_OBJ by default if the OBJDIR is writable. to Enable AUTO_OBJ by default if the OBJDIR is writable, only for in-tree builds..Nov 1 2017, 5:30 PM

bdrewery edited the summary of this revision. (Show Details)

bdrewery marked 2 inline comments as done.Nov 1 2017, 5:37 PM

bdrewery added inline comments.

share/mk/src.sys.obj.mk
107	Ah I missed you said "`mkdir -p` deals with this case sanely now". Ok we're on the same page.

bdrewery marked an inline comment as done.Nov 1 2017, 6:43 PM

bdrewery added inline comments.

share/mk/src.sys.obj.mk
123	Hm, I just realized this whole method is unnecessary. If the process can write (or even make any directory) to the last component before the first missing component, then it's a success. I.e., if /usr/obj is writable and /usr/obj/usr doesn't exist, that's good enough for a success. If /usr/obj is not writable and /usr/obj/usr doesn't exist, that's good enough for a failure.

bdrewery edited the summary of this revision. (Show Details)Nov 1 2017, 8:32 PM

Simplify detection, no need to create directories, just look for the last missing component and if we can write to that directory.
Simplify more of the change overall
Check the wanted OBJDIR rather than only OBJROOT
Support broken symlinks in the path by recursing on the new link's path, tested with 2 nested links in MAKEOBJDIRPREFIX both with and without missing dirs and writable dirs
Fix more of the 'make showconfig' support, it actually runs a child with make -dg1 so we need to export to that one
Add an UPDATING entry

Harbormaster completed remote builds in B12380: Diff 34632.Nov 1 2017, 8:35 PM

Remove leftover comment to myself

Harbormaster completed remote builds in B12381: Diff 34633.Nov 1 2017, 8:36 PM

As I said, recent FreeBSD has no problem with mkdir -p , but auto.obj.mk needs to work on lots of systems, and at least one - I can't recall now, did have a bug where mkdir -p had a race condition.

Any further comments? I'd like to commit this today.

A couple of nits but generally looks ok

share/mk/src.sys.obj.mk
149	fwiw you can remove the check for `.TARGETS == ""` by using `${.TARGETS:Uall:Nclean*.....} != ""`
162	I would normally use `[ ! -d`

This revision is now accepted and ready to land.Nov 2 2017, 5:07 PM

Thanks!

share/mk/src.sys.obj.mk
149	Sure I'll use that, I took the pattern from auto.obj.mk.

bdrewery marked an inline comment as done.Nov 2 2017, 5:12 PM

bdrewery added inline comments.

share/mk/src.sys.obj.mk
149	Actally .TARGETS:Uall doesn't work here as .TARGETS is defined blank for some reason at this point. ~/git/freebsd2 # make -C bin/sh -V .OBJDIR make: "/root/git/freebsd2/share/mk/src.sys.obj.mk" line 100: set '' From: .info ${.TARGETS:Dset} '${.TARGETS}'

Closed by commit rS325330: Enable AUTO_OBJ by default if the OBJDIR is writable, only for in-tree builds. (authored by bdrewery). · Explain WhyNov 2 2017, 6:09 PM

This revision was automatically updated to reflect the committed changes.

Enable AUTO_OBJ by default if the OBJDIR is writable, only for in-tree builds.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 34478

share/mk/src.sys.obj.mk

Enable AUTO_OBJ by default if the OBJDIR is writable, only for in-tree builds.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 34478

share/mk/src.sys.obj.mk

Enable AUTO_OBJ by default if the OBJDIR is writable, only for in-tree builds.
ClosedPublic
Actions

Revision Contents
Changeset List