-
-
Notifications
You must be signed in to change notification settings - Fork 16
Create distribution packages for ubuntu (.deb release v0.0.1) #129
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
base: main
Are you sure you want to change the base?
Conversation
inflection/CMakeLists.txt
Outdated
|
||
set(CPACK_PACKAGE_NAME "unicode-inflection") | ||
set(CPACK_PACKAGE_VENDOR "Unicode Consortium") | ||
set(CPACK_PACKAGE_CONTACT "grhoten@gmail.com") |
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 we need to find another contact. I'm not sure that I'm ready to have my email address 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.
Is that a mandatory field? If we have to have it, we can use the inflection-team@unicode.org one.
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’d prefer just listing the official GitHub project. The discussion page can be used for communication.
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.
Agree.
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.
Noted
inflection/CMakeLists.txt
Outdated
|
||
# DEB-specific options | ||
set(CPACK_DEBIAN_PACKAGE_MAINTAINER "Unicode Consortium <grhoten@gmail.com>") | ||
set(CPACK_DEBIAN_PACKAGE_DEPENDS "libicu-dev (>= 77.1)") |
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.
The dev part is only needed for building. The non-dev dependency is needed for runtime.
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'll update the dependency to libicu77 for runtime
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.
Looks good in general with some comments.
- 'v*' | ||
workflow_dispatch: | ||
pull_request: | ||
branches: [ main, master ] |
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 don't think we need the master part. It's obsolete and our repo doesn't have it.
with: | ||
lfs: true | ||
10000
|
||
- name: Setup Git LFS |
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.
We can land it as is for now, but fix it after Frank lands 128 (LFS caching).
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.
Sounds good.
sudo apt update | ||
sudo apt install -y cmake build-essential clang pkg-config | ||
|
||
- name: Install ICU 77.1 |
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.
ICU caching already landed - we can use it here to save bandwidth.
inflection/CMakeLists.txt
Outdated
|
||
set(CPACK_PACKAGE_NAME "unicode-inflection") | ||
set(CPACK_PACKAGE_VENDOR "Unicode Consortium") | ||
set(CPACK_PACKAGE_CONTACT "grhoten@gmail.com") |
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.
Is that a mandatory field? If we have to have it, we can use the inflection-team@unicode.org one.
- name: Install ICU 77.1 | ||
run: | | ||
cd /tmp | ||
wget https://github.com/unicode-org/icu/releases/download/release-77-1/icu4c-77_1-src.tgz |
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 download and compile sources? Why not just download and install binary release for Ubuntu? It takes time to compile ICU.
One needs to only copy the $download_location/usr tree to /usr/local/ folder
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.
See https://github.com/unicode-org/inflection/pull/128/files for example on how to do it.
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.
Thanks for the feedback, I’ll update the workflow to use the binary release and copy the $download_location/usr tree to /usr/local instead of building from source.
d526bf8
to
dde6c7a
Compare
dde6c7a
to
a22316b
Compare
tags: | ||
- 'v*' | ||
workflow_dispatch: | ||
pull_request: |
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.
Would this run the flow on each pull request? I don't think we need that.
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’ll remove the pull_request trigger so the workflow runs only on tag pushes and manual dispatch.
run: | | ||
sudo ldconfig | ||
- name: Debug ICU version |
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.
Do we need this for final version?
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 was helpful for initial testing, but I agree, it can be removed from the final version. I’ll delete the debug step.
- name: Run tests | ||
run: | | ||
cd inflection/build | ||
make check |
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 you can run test in parallel as with previous step (-j$...)
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.
Yes,
I’ll update the make check step to use -j$(nproc) for parallel test execution.
if: steps.cache-icu.outputs.cache-hit != 'true' | ||
run: | | ||
cd /tmp | ||
wget https://github.com/unicode-org/icu/releases/download/release-77-1/icu4c-77_1-Ubuntu22.04-x64.tgz |
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.
can we extract the ICU version into a variable and use in multiple places? Just the icu4c-77_1-Ubuntu22.04-x64.tgz part
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.
What about ARM64?
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.
Let me look into it, Currently, this workflow targets x64. I’ll explore adding runs-on: ubuntu-latest matrix support for arm64.
path: /usr/local | ||
key: icu-77.1-ubuntu-${{ runner.os }} | ||
|
||
- name: Install ICU 77.1 (Binary) |
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.
Pls remove the version from the name - this step will stay the same in the future, we'll just have to update the numbers below.
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’ll rename the step to something generic like Install ICU (Binary)
Is it possible to derive the version of the Debian package from resources/share/inflection/config.properties? |
Great suggestion, I'll look into it. |
Btw I asked LLM to do something like this, and it came close: CMake
Action with ICU caching:
|
inflection/CMakeLists.txt
Outdated
list(GET INFLECTION_VERSION_LIST 2 CPACK_PACKAGE_VERSION_PATCH) | ||
endif() | ||
|
||
|
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.
remove extra new lines (line 141 and 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.
Okay
set(CPACK_PACKAGE_NAME "unicode-inflection") | ||
set(CPACK_PACKAGE_VERSION "${INFLECTION_VERSION}") | ||
|
||
|
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 would put a comment here:
# Apply the current tagged Inflection version to the CPack version.
# CPack inherits the version from ??? so we need to reset before the assignment.
@@ -0,0 +1,94 @@ | |||
name: Build & Package (Ubuntu) |
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.
We should have a comment on top (or a separate document) that tells us how to update ICU dependancy:
# To update ICU version:
# 1. Update ICU_VERSION variable
# 2. Check package names, e.g. is it still Ubuntu22.04-
# 3. Update inflection/CMakeLists.txt where we check CPACK_DEBIAN_PACKAGE_DEPENDS "libicu77 (>= 77.1)"
Anything else I missed?
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 idea, will do that.
Ubuntu Packaging with CPack & GitHub Actions
This PR introduces a GitHub Actions workflow for building and packaging the Unicode Inflection library on Ubuntu using CPack. It generates
.deb
and.tar.gz
artifacts and uploads them to GitHub Releases when a Git tag is pushed. ICU 77.1 is installed from a prebuilt binary release.Key Changes
CMake / CPack Configuration
CMakeLists.txt
to:Inflection-0.1.0
).deb
and.tar.gz
artifactsGitHub Actions Workflow
.github/workflows/create-ubuntu-distribution-packaging.yml
v0.1.0
)make check
cpack
Outputs
.deb
package.tar.gz
package