Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Clean up Makefile #21031

Closed
wants to merge 6 commits into from
Closed

Clean up Makefile #21031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a6ab877
Clean up Makefile
khyperia Jul 21, 2017
a1f140c
Makefile cleanup: Unify some things with build.ps1
khyperia Jul 24, 2017
898462a
Disable warn-as-error for now
khyperia Jul 24, 2017
649d4e3
Include MSBuildTask in `bootstrap` build.
khyperia Jul 24, 2017
bf96ea4
Inline restore.sh for maintainability
khyperia Jul 25, 2017
3907851
Move runtime ID to msbuild arg instead of dotnet
khyperia Jul 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 60 additions & 45 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,69 +1,84 @@
SHELL = /usr/bin/env bash
OS_NAME = $(shell uname -s)
BUILD_CONFIGURATION = Debug
BINARIES_PATH = $(shell pwd)/Binaries
SRC_PATH = $(shell pwd)/src
BOOTSTRAP_PATH = $(BINARIES_PATH)/Bootstrap
BUILD_LOG_PATH =
HOME_DIR = $(shell cd ~ && pwd)
DOTNET_VERSION = 1.0.1
DOTNET = $(BINARIES_PATH)/dotnet-cli/dotnet
TARGET_FX = netcoreapp1.1
SHELL := /usr/bin/env bash
Copy link
Member

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 :=?

Copy link
Contributor Author

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.").

Copy link
Contributor Author

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)

Copy link
Member

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.

OS_NAME := $(shell uname -s)
BUILD_CONFIGURATION := Debug
# $(dir) ends with slash
THIS_MAKEFILE_PATH := $(dir $(realpath $(lastword $(MAKEFILE_LIST))))
BINARIES_PATH := $(THIS_MAKEFILE_PATH)Binaries
SRC_PATH := $(THIS_MAKEFILE_PATH)src
BOOTSTRAP_PATH := $(BINARIES_PATH)/Bootstrap
BUILD_LOG_PATH :=
DOTNET_VERSION := 1.0.1
DOTNET_PATH := $(BINARIES_PATH)/dotnet-cli
DOTNET := $(DOTNET_PATH)/dotnet
export PATH := $(DOTNET_PATH):$(PATH)
Copy link
Member

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?

Copy link
Contributor Author

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.)

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agocke Correct - Make docs "To pass down, or export, a variable, make adds the variable and its value to the environment for running each line of the recipe." (those docs are for sub-makes, there might be actual docs for export, but it gets the point across)


MSBUILD_ADDITIONALARGS := /v:m /fl /fileloggerparameters:Verbosity=normal /p:Configuration=$(BUILD_CONFIGURATION)
# Workaround, see https://github.com/dotnet/roslyn/issues/10210
ifeq ($(origin HOME), undefined)
# Note that while ~ usually refers to $HOME, in the case where $HOME is unset,
# it looks up the current user's home dir, which is what we want.
# https://www.gnu.org/software/bash/manual/html_node/Tilde-Expansion.html
export HOME := $(shell cd ~ && pwd)
endif

ifeq ($(OS_NAME),Linux)
RUNTIME_ID := $(shell . /etc/os-release && echo $$ID.$$VERSION_ID)-x64
RUNTIME_ID := $(shell . /etc/os-release && echo $$ID.$$VERSION_ID)-x64
else ifeq ($(OS_NAME),Darwin)
RUNTIME_ID := osx.10.12-x64
RUNTIME_ID := osx.10.12-x64
else
$(error "Unknown OS_NAME: $(OS_NAME)")
endif

MSBUILD_ARGS := /nologo '/consoleloggerparameters:Verbosity=minimal;summary' /p:Configuration=$(BUILD_CONFIGURATION)

ifneq ($(BUILD_LOG_PATH),)
MSBUILD_ADDITIONALARGS := $(MSBUILD_ADDITIONALARGS) /fileloggerparameters:LogFile=$(BUILD_LOG_PATH)
MSBUILD_ARGS += /filelogger '/fileloggerparameters:Verbosity=normal;logFile=$(BUILD_LOG_PATH)'
endif

MSBUILD_MAIN_ARGS := $(MSBUILD_ARGS)
MSBUILD_BOOTSTRAP_ARGS := $(MSBUILD_ARGS)
Copy link
Member

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

Copy link
Contributor Author

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.


MSBUILD_BOOTSTRAP_ARGS += /p:RuntimeIdentifier=$(RUNTIME_ID)

# This gets a bit complex. There are two cases here:
# BOOTSTRAP=false:
# Things proceed simply. The "all" target does not depend on the bootstrap
# target, so bootstrap is never built, and BootstrapBuildPath is unspecified.
# BOOTSTRAP=true:
# BOOTSTRAP_DEPENDENCY is set to "bootstrap", making the "all" target depend
# on it, and so the bootstrap compiler gets built. Additionally,
# BootstrapBuildPath is specified, but *only* for the main build, *not* the
# bootstrap build.
ifeq ($(BOOTSTRAP),true)
MSBUILD_ARGS = $(MSBUILD_ADDITIONALARGS) /p:CscToolPath=$(BOOTSTRAP_PATH)/csc /p:CscToolExe=csc /p:VbcToolPath=$(BOOTSTRAP_PATH)/vbc /p:VbcToolExe=vbc
# MSBUILD_MAIN_ARGS += /p:BootstrapBuildPath=$(BOOTSTRAP_PATH)
MSBUILD_MAIN_ARGS += /p:CscToolPath=$(BOOTSTRAP_PATH)/csc /p:CscToolExe=csc /p:VbcToolPath=$(BOOTSTRAP_PATH)/vbc /p:VbcToolExe=vbc
BOOTSTRAP_DEPENDENCY := bootstrap
else
MSBUILD_ARGS = $(MSBUILD_ADDITIONALARGS)
BOOTSTRAP_DEPENDENCY :=
endif

BUILD_CMD = dotnet build $(MSBUILD_ARGS)

.PHONY: all bootstrap test toolset
.PHONY: all bootstrap test restore

all: restore
@export PATH="$(BINARIES_PATH)/dotnet-cli:$(PATH)" ; \
export HOME="$(HOME_DIR)" ; \
$(BUILD_CMD) CrossPlatform.sln
all: restore $(BOOTSTRAP_DEPENDENCY)
@echo Building CrossPlatform.sln
dotnet build $(THIS_MAKEFILE_PATH)CrossPlatform.sln $(MSBUILD_MAIN_ARGS)

bootstrap: restore
export HOME="$(HOME_DIR)" ; \
export PATH="$(BINARIES_PATH)/dotnet-cli:$(PATH)" ; \
$(BUILD_CMD) src/Compilers/CSharp/CscCore && \
$(BUILD_CMD) src/Compilers/VisualBasic/VbcCore && \
mkdir -p $(BOOTSTRAP_PATH)/csc && mkdir -p $(BOOTSTRAP_PATH)/vbc && \
dotnet publish -c $(BUILD_CONFIGURATION) -r $(RUNTIME_ID) src/Compilers/CSharp/CscCore -o $(BOOTSTRAP_PATH)/csc && \
dotnet publish -c $(BUILD_CONFIGURATION) -r $(RUNTIME_ID) src/Compilers/VisualBasic/VbcCore -o $(BOOTSTRAP_PATH)/vbc
rm -rf Binaries/$(BUILD_CONFIGURATION)
@echo Building Bootstrap
dotnet publish $(SRC_PATH)/Compilers/CSharp/CscCore -o $(BOOTSTRAP_PATH)/csc $(MSBUILD_BOOTSTRAP_ARGS)
dotnet publish $(SRC_PATH)/Compilers/VisualBasic/VbcCore -o $(BOOTSTRAP_PATH)/vbc $(MSBUILD_BOOTSTRAP_ARGS)
rm -rf $(BINARIES_PATH)/$(BUILD_CONFIGURATION)

test:
@export PATH="$(BINARIES_PATH)/dotnet-cli:$(PATH)" ; \
export HOME="$(HOME_DIR)" ; \
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 $(SRC_PATH)/Test/DeployCoreClrTestRuntime -o $(BINARIES_PATH)/$(BUILD_CONFIGURATION)/CoreClrTest -r $(RUNTIME_ID) -p:RoslynRuntimeIdentifier=$(RUNTIME_ID) $(MSBUILD_MAIN_ARGS)
$(THIS_MAKEFILE_PATH)build/scripts/tests.sh $(BUILD_CONFIGURATION)

restore: $(DOTNET)
export PATH="$(BINARIES_PATH)/dotnet-cli:$(PATH)" ; \
./build/scripts/restore.sh
dotnet restore -v Minimal --disable-parallel $(THIS_MAKEFILE_PATH)build/ToolsetPackages/BaseToolset.csproj
dotnet restore -v Minimal --disable-parallel $(THIS_MAKEFILE_PATH)CrossPlatform.sln

$(DOTNET):
mkdir -p $(BINARIES_PATH) ; \
pushd $(BINARIES_PATH) ; \
curl -O https://raw.githubusercontent.com/dotnet/cli/rel/1.0.0/scripts/obtain/dotnet-install.sh && \
chmod +x dotnet-install.sh && \
./dotnet-install.sh --version "$(DOTNET_VERSION)" --install-dir "$(BINARIES_PATH)/dotnet-cli"

curl https://raw.githubusercontent.com/dotnet/cli/rel/$(DOTNET_VERSION)/scripts/obtain/dotnet-install.sh | \
$(SHELL) -s -- --version "$(DOTNET_VERSION)" --install-dir "$(DOTNET_PATH)"

clean:
@rm -rf Binaries
rm -rf $(BINARIES_PATH)
12 changes: 0 additions & 12 deletions build/scripts/restore.sh
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions build/scripts/tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@

BUILD_CONFIGURATION=$1

# This function will update the PATH variable to put the desired
# version of Mono ahead of the system one.

cd Binaries/$BUILD_CONFIGURATION/CoreClrTest

chmod +x ./corerun
Expand Down
17 changes: 3 additions & 14 deletions cibuild.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,13 @@ usage()
echo " --debug Build Debug (default)"
echo " --release Build Release"
echo " --skiptest Do not run tests"
echo " --skipcrossgen Do not crossgen the bootstrapped compiler"
echo " --skipcommitprinting Do not print commit information"
echo " --nocache Force download of toolsets"
}

BUILD_CONFIGURATION=Debug
USE_CACHE=true
SKIP_TESTS=false
SKIP_CROSSGEN=false
SKIP_COMMIT_PRINTING=false

MAKE="make"
Expand Down Expand Up @@ -59,10 +57,6 @@ do
SKIP_TESTS=true
shift 1
;;
--skipcrossgen)
SKIP_CROSSGEN=true
shift 1
;;
--skipcommitprinting)
SKIP_COMMIT_PRINTING=true
shift 1
Expand All @@ -74,8 +68,6 @@ do
esac
done

MAKE_ARGS="BUILD_CONFIGURATION=$BUILD_CONFIGURATION SKIP_CROSSGEN=$SKIP_CROSSGEN"

if [ "$CLEAN_RUN" == "true" ]; then
echo Clean out the enlistment
git clean -dxf .
Expand All @@ -86,13 +78,10 @@ if [ "$SKIP_COMMIT_PRINTING" == "false" ]; then
git show --no-patch --pretty=raw HEAD
fi

echo Building Bootstrap
$MAKE bootstrap $MAKE_ARGS

echo Building CrossPlatform.sln
$MAKE all $MAKE_ARGS BOOTSTRAP=true BUILD_LOG_PATH=Binaries/Build.log
MAKE_TARGET="all"

if [ "$SKIP_TESTS" == "false" ]; then
$MAKE test $MAKE_ARGS
MAKE_TARGET="$MAKE_TARGET test"
fi

$MAKE $MAKE_TARGET BUILD_CONFIGURATION=$BUILD_CONFIGURATION BOOTSTRAP=true BUILD_LOG_PATH=Binaries/Build.log