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 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 9 additions & 17 deletions docs/workflow/requirements/linux-requirements.md
Expand Up @@ -22,23 +22,15 @@ Minimum RAM required to build is 1GB. The build is known to fail on 512 MB VMs (

### Toolchain Setup

Install the following packages for the toolchain:

* CMake 3.20 or newer
* llvm
* lld
* clang
* build-essential
* python-is-python3
* curl
* git
* lldb
* libicu-dev
* liblttng-ust-dev
* libssl-dev
* 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.


You can install all the above dependencies by running

```bash
sudo ./eng/install-native-dependencies.sh
```

### Community-supported environments

**NOTE**: If you have an Ubuntu version older than 22.04 LTS, or Debian version older than 12, don't install `cmake` using `apt` directly. Follow the note written down below.

Expand Down
19 changes: 19 additions & 0 deletions eng/debian-reqs.txt
@@ -0,0 +1,19 @@
build-essential
clang
cmake
curl
gettext
git
libicu-dev
libkrb5-dev
liblldb-dev
liblttng-ust-dev
libssl-dev
libunwind8-dev
lld
lldb
llvm
locales
ninja-build
python-is-python3
zlib1g-dev
5 changes: 3 additions & 2 deletions eng/install-native-dependencies.sh
Expand Up @@ -11,20 +11,21 @@ set -e
os="$(echo "$1" | tr "[:upper:]" "[:lower:]")"

if [ -z "$os" ]; then
# shellcheck source-path=SCRIPTDIR
. "$(dirname "$0")"/common/native/init-os-and-arch.sh
fi

case "$os" in
linux)
if [ -e /etc/os-release ]; then
# shellcheck source=/dev/null
. /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
agocke marked this conversation as resolved.
Show resolved Hide resolved
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.

Copy link
Member Author

@agocke agocke May 22, 2024

Choose a reason for hiding this comment

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

Yeah brewfile was what I was copying in spirit. I'm hoping this is useful for others -- if it's not we can fold back into the script.


localedef -i en_US -c -f UTF-8 -A /usr/share/locale/locale.alias en_US.UTF-8
elif [ "$ID" = "alpine" ]; then
Expand Down
3 changes: 3 additions & 0 deletions eng/native/build-commons.sh
Expand Up @@ -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.

fi

if [[ "$__UseNinja" == 1 ]]; then
Expand Down