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

Add more complete linux build instructions #101631

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

agocke
Copy link
Member

@agocke agocke commented Apr 26, 2024

I'm trying to make it slightly easier to install the needed requirements, and add some validation if a user hasn't installed the requirements.

Also, I validated that these instructions still work for Ubuntu 24.04 and have noted that other installs are only community-supported.

I'm trying to make it slightly easier to install the needed requirements, and add some validation if a user hasn't installed the requirements.

Also, I validated that these instructions still work for Ubuntu 24.04 and have noted that other installs are only community-supported.
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 26, 2024
@am11
Copy link
Member

am11 commented Apr 26, 2024

I'm using eng/install-native-dependencies.sh in Debian/Ubuntu, Alpine and macOS. It works well. Do we need another mechanism; another list to maintain?

eng/native/build-commons.sh Outdated Show resolved Hide resolved
@jkotas jkotas added area-Infrastructure and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 27, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

@agocke
Copy link
Member Author

agocke commented Apr 29, 2024

I'm using eng/install-native-dependencies.sh in Debian/Ubuntu, Alpine and macOS. It works well. Do we need another mechanism; another list to maintain?

I didn't know about that entry point. I'm happy to integrate with that.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@am11
Copy link
Member

am11 commented Apr 29, 2024

I'm happy to integrate with that.

I think it would be good to just point to the (self-documenting) helper script. On fresh machine (baremetal, cloud VM, CI, container) it installs the prereqs in a few second enough to build all default subsets.

# replace 'ubuntu' with 'alpine' without changing the steps
$ docker run --rm -v$(pwd):/runtime -w /runtime --platform linux/arm64/v8 ubuntu sh -c '
	eng/install-native-dependencies.sh;
	./build.sh -c Release'

@agocke
Copy link
Member Author

agocke commented Apr 29, 2024

I think it would be good to just point to the (self-documenting) helper script

I'm fine with this but the lists are currently inconsistent.

Our docs:

build-essential
clang
cmake
curl
git
libicu-dev
libkrb5-dev
liblttng-ust-dev
libssl-dev
lld
lldb
llvm
ninja-build
python-is-python3
zlib1g-dev

Script

build-essential
clang
cmake
gettext
libicu-dev
libkrb5-dev
liblldb-dev
liblttng-ust-dev
libssl-dev
libunwind8-dev
lldb
llvm
locales
zlib1g-dev

Diff:

build-essential
clang
cmake
-curl
-git
+gettext
libicu-dev
libkrb5-dev
+liblldb-dev
liblttng-ust-dev
libssl-dev
-lld
+libunwind8-dev
lldb
llvm
-ninja-build
-python-is-python3
+locales
zlib1g-dev

Thoughts? I personally think ninja-build should be in there even if it's not strictly necessary.

@am11
Copy link
Member

am11 commented Apr 29, 2024

Yup, agreed.

  • ninja-build can be added in both debian and alpine (it has the same package name).
  • curl and python3 on debian, and python3 on alpine are implicitly installed (but we can explicitly specify them)
  • lld can be added in both debian and alpine.
  • git is not strictly needed (but can be added).

@agocke
Copy link
Member Author

agocke commented Apr 30, 2024

OK, I took the most conservative approach of just taking the union, since I'm not sure what's actually necessary.

Also, docs have been updated to point to the shell script.

@@ -39,6 +39,9 @@ check_prereqs()
# We try again with the PKG_CONFIG_PATH in place, if pkg-config still can't find OpenSSL, exit with an error, cmake won't find OpenSSL either
pkg-config openssl || { echo >&2 "Please install openssl before running this script, see https://github.com/dotnet/runtime/blob/main/docs/workflow/requirements/macos-requirements.md"; exit 1; }
fi
elif [[ "$__HostOS" == "linux" ]]; then
# Check presence of cmake on the path
command -v cmake 2>/dev/null || { echo >&2 "Please install cmake before running this script, see https://github.com/dotnet/runtime/blob/main/docs/workflow/requirements/linux-requirements.md"; exit 1; }
Copy link
Member

Choose a reason for hiding this comment

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

We are already testing it in gen-buildsys.sh. Also why is this linux only?

Copy link
Member Author

Choose a reason for hiding this comment

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

This check is happening earlier and is clearer. Ideally I'd like this check to happen even earlier, but I think that belongs in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

We used to have an upfront check for a while and it was deleted in #39044.

I think if we are bringing it back, we should do it for all platforms, not just linux. CMake is needed on all platforms alike.

Copy link
Member Author

Choose a reason for hiding this comment

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

And this is linux-only because I think checking for pkg-config is better on Mac. What we're looking for is the thing least-likely to have been installed by-default.

Copy link
Member

Choose a reason for hiding this comment

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

If we use else instead of elif, that will cover mac+linux+{everything-else}:

else
  # Check presence of cmake on the path
  command -v cmake 2>/dev/null || { echo >&2 "Please install cmake before running this script, see https://github.com/dotnet/runtime/tree/main/docs/workflow/requirements"; exit 1; }

User can then read {platform}-instructions.md.

Copy link
Member

Choose a reason for hiding this comment

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

This check is happening earlier and is clearer.

It is rendering this check redundant which can be deleted:

if ! cmake_command=$(command -v cmake); then
echo "CMake was not found in PATH."

Copy link
Member Author

Choose a reason for hiding this comment

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

We used to have an upfront check for a while and it was deleted in #39044.

Not quite the same -- that's a version check, here I'm just doing a basic check to see if it exists at all. The intent is to catch people who have not installed the prereqs at all, not people who have misconfigured prereqs.

* libkrb5-dev
* zlib1g-dev
* ninja-build (optional, enables building native code with ninja instead of make)
Install the packages listed in [debian-reqs.txt](/eng/debian-reqs.txt).
Copy link
Member

Choose a reason for hiding this comment

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

/eng/debian-reqs.txt no longer exists

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that's right. Now I remembered why I structured it like this: so that users could easily see exactly what they were installing without having to scan a bash script and also without having the risk of the list going out of date. I prefer my previous solution. I'm going to revert the last commit.

. /etc/os-release
fi

if [ "$ID" = "debian" ] || [ "$ID_LIKE" = "debian" ]; then
apt update

apt install -y build-essential gettext locales cmake llvm clang lldb liblldb-dev libunwind8-dev libicu-dev liblttng-ust-dev \
libssl-dev libkrb5-dev zlib1g-dev
xargs apt-get install -y < eng/debian-reqs.txt
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
xargs apt-get install -y < eng/debian-reqs.txt
apt install -y build-essential gettext locales cmake llvm clang lldb liblldb-dev libunwind8-dev libicu-dev liblttng-ust-dev \
libssl-dev libkrb5-dev zlib1g-dev

lets keep them separate then. I was proposing to deduplicate it the other way around. Makes less sense to change it for one distro leaving others on the table.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you elaborate? Ubuntu is the only Linux dev platform we "officially" support developing on. Wouldn't it make sense to go through more work to make it a good "reference" platform?

Copy link
Member

Choose a reason for hiding this comment

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

This script is idempotent in nature; as it just calls package managers of that nature (worst case: print "package is already installed"; exit 0). It supports multiple platforms and has room for more. In the context of this script, there is no official precedence. Currently, it is providing support for Ubuntu, Alpine and Apple platforms.

The suggestion above was to just point people to the usage of this self-documenting script regardless of the distro. If/when someone will hit Unsupported distro. distro: $ID branch, they will likely get encouraged to add support for their platform.

Unless we think listing the dependencies for Debian/Ubuntu (without further description) separately is meaningful, IMO, it's better to keep it simple. On the other hand, seeing that we have eng/Brewfile also separated (as opposed to brew install <the whole grocery list>), we can probably live with this separate file as well. 😅

If we are going to use the separate version, please make sure to use absolute path (so it can be called from any location as before): agocke/runtime@automated-workflow...am11:runtime:patch-35.

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

Successfully merging this pull request may close these issues.

None yet

4 participants