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.

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

Makefile tweaks #1870

Merged
merged 1 commit into from
Jun 25, 2018
Merged

Makefile tweaks #1870

merged 1 commit into from
Jun 25, 2018

Conversation

Rosalie241
Copy link
Contributor

No description provided.

Makefile Outdated
for m in $BINFMT_MISC_TROUBLE; do \
if [ -f /proc/sys/fs/binfmt_misc/$$m ]; then \
for m in ${BINFMT_MISC_TROUBLE}; do \
if [ -f "/proc/sys/fs/binfmt_misc/$m" ]; then \
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be $$m, not $m.

Makefile Outdated
@@ -1,12 +1,12 @@
export OS_NAME := $(shell uname)
Copy link
Member

Choose a reason for hiding this comment

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

I don't find it to be an "improvement" to have one export declaration for all variables instead of one per 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.

it's personal preference but it makes the Makefile look more consistent at the start, besides exporting variables when they're not needed is also unnecessary. So should I revert this change or not?

Copy link
Member

Choose a reason for hiding this comment

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

The only variable which doesn't appear to need the export is $(OS_NAME); both $(OS_ARCH) and $(NO_SUDO) may be used by child processes.

Revert the change; it won't matter if we export $(OS_NAME).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, reverted

Makefile Outdated
-mkdir -p "$(prefix)/lib/mono/xbuild-frameworks"
-mkdir -p "$(prefix)/lib/xamarin.android"
-mkdir -p "$(prefix)/lib/mono/xbuild/Xamarin/"
-mkdir -p "$(prefix)/bin" "$(prefix)/lib/mono/xbuild-frameworks" \
Copy link
Member

Choose a reason for hiding this comment

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

I also don't find it to be an improvement to go from one directory per line to requiring \.

@jonpryor
Copy link
Member

build

@jonpryor jonpryor merged commit 8baa43d into dotnet:master Jun 25, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Feb 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants