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

Pass through $PATH to find dotnet #685

Merged
merged 1 commit into from
May 11, 2022
Merged

Pass through $PATH to find dotnet #685

merged 1 commit into from
May 11, 2022

Conversation

easwarh
Copy link
Contributor

@easwarh easwarh commented May 3, 2022

This commit makes it easier to build on Linux from source. Related to #606, but
applies to x64 builds as well.

Signed-off-by: Easwar Hariharan [email protected]

Copy link
Collaborator

@mjcheetham mjcheetham left a comment

Choose a reason for hiding this comment

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

Hello 👋!

Thanks for the contribution. I'm in favour of making it easier to build GCM on more platforms, but I have one concern about this change as it stands.

The $DOTNET_ROOT variable is set by the dotnet app host executable, which is typically the caller of these scripts (via MSBuild). However in the cases that this script is called directly (not via dotnet) then the variable is not set, and the commands being run in this script will be /dotnet publish ... which will not resolve correctly.

If we are to accept this then there should be a check that if the $DOTNET_ROOT variable is not set, then the script should fall back to invoking dotnet and let the shell resolve the executable location..
..OR add something like this to explicitly resolve:

if [ -z "$DOTNET_ROOT" ]; then
    DOTNET_ROOT="$(dirname $(which dotnet))"
fi

@easwarh
Copy link
Contributor Author

easwarh commented May 4, 2022

Hi @mjcheetham, that's a great call out, thanks for the feedback! I'll fix this up, would you rather have a fixup commit (commit --fixup), a new commit, or an amendment to the existing commit (commit --amend)?

@easwarh easwarh requested a review from mjcheetham May 5, 2022 01:15
@easwarh easwarh changed the title Pass through DOTNET_ROOT and use it as prefix to find dotnet Pass through $PATH to find dotnet May 6, 2022
@ldennington
Copy link
Contributor

@easwarh - I know you directed your question at Matthew, but FWIW my usual workflow in GCM is to amend my existing commits.

@easwarh
Copy link
Contributor Author

easwarh commented May 11, 2022

@easwarh - I know you directed your question at Matthew, but FWIW my usual workflow in GCM is to amend my existing commits.

Thanks for the headsup. I'll push a merged commit.

Signed-off-by: Easwar Hariharan <[email protected]>
Copy link
Collaborator

@mjcheetham mjcheetham left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@mjcheetham mjcheetham merged commit 46d1b3c into git-ecosystem:main May 11, 2022
@easwarh easwarh deleted the eahariha/linux-src-build-script branch May 11, 2022 16:50
@ldennington ldennington mentioned this pull request Jun 15, 2022
# Publish core application executables
echo "Publishing core application..."
dotnet publish "$GCM_SRC" \
$DOTNET_ROOT/dotnet publish "$GCM_SRC" \

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants