-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up Makefile #21031
Clean up Makefile #21031
Conversation
DOTNET_VERSION = 1.0.1 | ||
DOTNET = $(BINARIES_PATH)/dotnet-cli/dotnet | ||
TARGET_FX = netcoreapp1.1 | ||
SHELL := /usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the distinction here between =
and :=
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=
is lazy, :=
is strict.
foo = hi $(bar)
bar = one
$(info $(foo))
bar = two
$(info $(foo))
prints "hi one" "hi two"
foo := hi $(bar)
bar := one
$(info $(foo))
bar := two
$(info $(foo))
prints "hi " "hi " (empty var).
The first behavior seems incredibly insane, so having the default for most variable assignments be :=
is good style.
Note it gets weird with recursive variables (e.g. our use of MSBUILD_ARGS
):
MSBUILD_ARGS = $(MSBUILD_ARGS) /some:switch
makes things get upset ("Recursive variable 'MSBUILD_ARGS' references itself (eventually). Stop.
").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(so the recursive case must use :=
)
Also, btw, in my list of changes, I listed this as "Use immediate sets instead of recursive sets in many places" (which is the documented name of each type of assignment - recursive is also called lazy, deferred, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah ... def seems like we should be using :=
then.
DOTNET_VERSION := 1.0.1 | ||
DOTNET_PATH := $(BINARIES_PATH)/dotnet-cli | ||
DOTNET := $(DOTNET_PATH)/dotnet | ||
export PATH := $(DOTNET_PATH):$(PATH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will change the $PATH
of all executed processes correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. However, we were setting PATH
in every rule anyway (export PATH="$(BINARIES_PATH)/dotnet-cli:$(PATH)"
at the start of every rule), so this has no effect. (Except, technically, the $(DOTNET):
rule, but that creates DOTNET_PATH
, so there's no harm on having it already there. Also clean
I guess, but that's just an rm
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume the export
turns this into a real environment variable instead of just a make variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
DOTNET_VERSION := 1.0.1 | ||
DOTNET_PATH := $(BINARIES_PATH)/dotnet-cli | ||
DOTNET := $(DOTNET_PATH)/dotnet | ||
export PATH := $(DOTNET_PATH):$(PATH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume the export
turns this into a real environment variable instead of just a make variable?
DOTNET_VERSION = 1.0.1 | ||
DOTNET = $(BINARIES_PATH)/dotnet-cli/dotnet | ||
TARGET_FX = netcoreapp1.1 | ||
SHELL := /usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah ... def seems like we should be using :=
then.
Makefile
Outdated
dotnet publish -r $(RUNTIME_ID) src/Test/DeployCoreClrTestRuntime -o $(BINARIES_PATH)/$(BUILD_CONFIGURATION)/CoreClrTest -p:RoslynRuntimeIdentifier=$(RUNTIME_ID) && \ | ||
build/scripts/tests.sh $(BUILD_CONFIGURATION) | ||
dotnet publish -r $(RUNTIME_ID) $(SRC_PATH)/Test/DeployCoreClrTestRuntime -o $(BINARIES_PATH)/$(BUILD_CONFIGURATION)/CoreClrTest -p:RoslynRuntimeIdentifier=$(RUNTIME_ID) | ||
./build/scripts/tests.sh $(BUILD_CONFIGURATION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the distinction of ./
do here vs. the old one? At a glance it seems to have same behavior: use build/scripts/tests.sh
from the current directory (which I'm always afraid may not be the directory of the make file). Does this add some other subtle distinction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No difference. Changed it for consistency: nearby we invoke ./build/scripts/restore.sh
. I'll update both to use $(THIS_MAKEFILE_PATH)
(which apparently ends with a slash, didn't read the Makefile docs for dir
well enough, I'll update that too), and any other relative-to-pwd things I find.
Ping @jaredpar @agocke - pushed another change. List of things:
|
debd205
to
2d7ae9c
Compare
Update: apparently |
2d7ae9c
to
03c762f
Compare
Correct. That flag controls how they re-use the set of spawned MSBuild processes. That's not implemented at all on Linux yet. |
Also root a couple paths, and modify restore.sh to be consistent. (without restore.sh change, build can fail with "too many files open")
03c762f
to
a1f140c
Compare
There is an existing issue that warn-as-error surfaced - analyzer DLLs are getting loaded twice.
Makefile
Outdated
@@ -29,7 +29,7 @@ else | |||
$(error "Unknown OS_NAME: $(OS_NAME)") | |||
endif | |||
|
|||
MSBUILD_ARGS := /p:TreatWarningsAsErrors=true /warnaserror /nologo '/consoleloggerparameters:Verbosity=minimal;summary' /p:Configuration=$(BUILD_CONFIGURATION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you file a bug to track us turning this back on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
90bbdc0
to
008f783
Compare
Makefile
Outdated
dotnet publish -c $(BUILD_CONFIGURATION) -r $(RUNTIME_ID) src/Compilers/VisualBasic/VbcCore -o $(BOOTSTRAP_PATH)/vbc | ||
rm -rf Binaries/$(BUILD_CONFIGURATION) | ||
@echo Building Bootstrap | ||
$(BOOTSTRAP_CMD) $(SRC_PATH)/Compilers/CSharp/CscCore -o $(BOOTSTRAP_PATH)/csc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't -o
a dotnet build
parameter, while the rest are msbuild
parameters? Maybe we should try to keep the dotnet build
parameters first, since I don't know how dotnet build
decides to pass parameters to msbuild.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - exactly what I just fixed (I struggled for a while trying to figure out how the heck it decides to pass things to msbuild - sorry for confusion for reviewers, I've been force-pushing a lot to re-run Jenkins tests. I just made an Ubuntu VM, though, so I can actually test locally now.).
The exact docs/syntax for dotnet build
says Usage: dotnet build [arguments] [options] [args]
which is... pretty much the most helpful wording ever.
- "arguments" means "the project file"
- "options" means "options understood by dotnet, e.g.
-o
" - "args" means "arguments passed through to msbuild"
The latest commits fix this ordering.
008f783
to
a7b6bf6
Compare
Also restructure Makefile to only be invoked once when bootstrapping/building/testing.
a7b6bf6
to
649d4e3
Compare
Also revert to CscToolPath/etc and remove MSBuildTask (fixing this would be a much more invasive change then Makefile refactoring)
endif | ||
|
||
MSBUILD_MAIN_ARGS := $(MSBUILD_ARGS) | ||
MSBUILD_BOOTSTRAP_ARGS := $(MSBUILD_ARGS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kind of confusing to put dotnet build
args in a list called MSBUILD_BOOTSTRAP_ARGS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went to split this into MSBUILD_ARGS and DOTNET_ARGS, but then I found that the -r
flag is just a simple passthrough to /p:RuntimeIdentifier=...
. I changed it to use that, because we have similar things with /p:RoslynRuntimeIdentifier
, and it's less confusing to have them be unified syntax.
The dotnet implementation is a simple pass-through to the parameter. Also, remove TARGET_FX, as it's unused.
Replaced by #21146, this PR is no longer relevant. |
Every time I opened up the Makefile, I got really sad. This cleans it up to be a tad more reasonable.
List of changes:
$(shell pwd)
is the same thing as the location of the Makefile (use MAKEFILE_LIST instead)HOME_DIR = $(shell cd ~ && pwd)
more clear with docs, and be more sane about setting it (and use Make's env-setting command, instead of copypastingHOME=$(HOME_DIR)
everywhere)PATH
(again, use Make's export command, instead of copy-pastingPATH=...
)MSBUILD_ADDITIONALARGS
was (just useMSBUILD_ARGS
)OS_NAME
is unknown, instead of failing obscurely laterpushd
and single huge commands passed to the shellSRC_PATH
and then never using it...)dotnet-install.sh
into bash, instead of doing wacky writes to disk (where we have to worry aboutchmod +x
and the current directory)HOME
bizzare-ness fromrestore.sh
, as we're setting that in the makefiletests.sh
I'm still really suspicious of strange things happening in the
bootstrap
rule, but that seems like a very invasive and potentially behavior-changing change, so I've left that for now.Using
pr-for-personal-review-only
until I take a look at ci results to make sure nothing is terribly broken.