-
Notifications
You must be signed in to change notification settings - Fork 92
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
Build .deb and .rpm packages for Linux #2182
Conversation
// --- Start Positron --- | ||
const FAIL_BUILD_FOR_NEW_DEPENDENCIES = false; | ||
// --- End Positron --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only change that was needed to get the build to pass. This allows the list of dependencies on shared libraries to be different from what MS expects on their build machine.
One difference is that we now depend on the C++ runtime via Zeromq. Hopefully that won't cause ABI issues for users of our packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add this info in a comment after Start Positron
so we know what changed if this generates a merge conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, done.
build_number: | ||
required: false | ||
description: "The build distance number only, e.g. 123" | ||
default: ${{ github.sha }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a number so defaulting it to a SHA doesn't seem right. Maybe 0 would be a better default, or 999 so we know it's coming from here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I also update https://github.com/posit-dev/positron/blob/main/.github/workflows/build-release-macos.yml from which I copied this section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to but now that you point it out it doesn't make sense there either! Apologies for the misleading precedent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the defaults in all workflows so they are consistent.
short_version: | ||
required: true | ||
description: "The short version number, including the build distance, e.g. 2023.12.0-123" | ||
default: ${{ github.sha }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same deal here w/r/t defaulting to a SHA
- name: Setup Build Environment | ||
run: | | ||
sudo apt-get update | ||
sudo apt-get install -y vim curl build-essential clang make cmake git r-base-dev python3-pip python-is-python3 libsodium-dev libxkbfile-dev pkg-config libsecret-1-dev libxss1 dbus xvfb libgtk-3-0 libgbm1 libnss3 libnspr4 libasound2 libkrb5-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need R to build Positron? (I think only Amalthea needs R to build?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely from our linux-based CI workflow is stale and the inspiration for this? https://github.com/posit-dev/positron/blob/main/.github/workflows/positron-ci.yml#L26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that's an artifact from the days when we built and tested Amalthea as part of the main Positron CI job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed R from the build-linux and positron-ci workflows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think only Amalthea needs R to build
@jmcphers, psh not anymore we don't!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🏆
// --- Start Positron --- | ||
const FAIL_BUILD_FOR_NEW_DEPENDENCIES = false; | ||
// --- End Positron --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add this info in a comment after Start Positron
so we know what changed if this generates a merge conflict?
Work around actions/upload-artifact#485
f106464
to
a0e93d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Addresses #1619
This Linux support benefits from:
VSCode's own infrastructure for building .deb and .rpm packages for Linux.
The detection of R installations via
/usr/bin/R
and/usr/local/bin/R
that we already had in place for macOS. These are the common places where R would be installed on Linux.As of ark 0.1.50 we now build a Linux x64 binary (see Add workflow to build .deb and .rpm binaries ark#227) that is included in the .deb and .rpm packages via the regular
install-kernel.ts
mechanism.Davis' work on linking R to Ark at runtime rather than at load time (Implement
libr
crate for dynamic runtime symbol resolution ark#205).