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

fix(x11/rnote): Fix compilation #21524

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(x11/rnote): Fix compilation #21524

wants to merge 3 commits into from

Conversation

EDLLT
Copy link

@EDLLT EDLLT commented Sep 20, 2024

Fixes #17655

Thanks @Biswa96 @TomJo2000 @Doublonmousse @romanovj @askorbinovaya-kislota and everyone else for your help

@EDLLT
Copy link
Author

EDLLT commented Sep 20, 2024

Seems like it needs tabs for indentation
I need to sleep now, I'll probably fix it tmrw

Copy link
Member

@TomJo2000 TomJo2000 left a comment

Choose a reason for hiding this comment

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

This looks very close to being done,
I do have a couple housekeeping items we need to take care of though.


(This is a pre-written, saved reply.)
Also please make sure to keep your commits squashed.
For adding to a single commit you can use git commit --amend.
Since you already have multiple commits on your branch though,
you'll need to squash those with git rebase -i HEAD~<n> first.
(Where <n> is the number of commits you want to modify.
Please make sure to only modify your commits.)

https://www.baeldung.com/ops/git-squash-commits#1-squash-the-last-x-commits

Since squashing or amending commits changes the git history you will need to force push any such changes.
e.g. git push --force,
or preferably git push --force-with-lease=branchname
to make sure you aren't clobbering any refs you haven't fetched locally yet.

x11-packages/rnote/build.sh Show resolved Hide resolved
local _version="0.21.4"
local _url="https://github.com/gettext-rs/gettext-rs/archive/refs/tags/gettext-sys-$_version.tar.gz"
local _path="$TERMUX_PKG_CACHEDIR/gettext-v$_version.tar.gz"
termux_download "$_url" "$_path" SKIP_CHECKSUM
Copy link
Member

Choose a reason for hiding this comment

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

Since we're downloading from a Github release tarball, please include the checksum.

Suggested change
termux_download "$_url" "$_path" SKIP_CHECKSUM
_sha256="211773408ab61880b94a0ea680785fc21fad307cd42594d547cf5a056627fcda"
termux_download "$_url" "$_path" "$_sha256"


# Fetch latest and greatest gettext from upstream and place it inside gettext-source/gettext-sys
local _upstream_source_version="0.22.5"
wget "https://mirrors.kernel.org/gnu/gettext/gettext-${_upstream_source_version}.tar.xz" -P "$TERMUX_PKG_SRCDIR/gettext-source/gettext-sys"
Copy link
Member

Choose a reason for hiding this comment

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

Please use termux_download here as well.


__patch_gettext_rs() {
# Patch gettext-rs crate to use a newer gettext version because the old one doesn't compile properly with clang
patch -p1 -d "$TERMUX_PKG_SRCDIR/gettext-source" < "$TERMUX_PKG_BUILDER_DIR"/gettext-rs-crate-patch.diff
Copy link
Member

Choose a reason for hiding this comment

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

Use -i to specify the input patch file instead of redirecting it in.

Bash would be an example for this usage.

PATCH_CHECKSUMS[032]=7b9c77daeca93ff711781d7537234166e83ed9835ce1ee7dcd5742319c372a16
for PATCH_NUM in $(seq -f '%03g' ${_PATCH_VERSION}); do
PATCHFILE=$TERMUX_PKG_CACHEDIR/bash_patch_${PATCH_NUM}.patch
termux_download \
"https://mirrors.kernel.org/gnu/bash/bash-${_MAIN_VERSION}-patches/bash${_MAIN_VERSION/./}-$PATCH_NUM" \
$PATCHFILE \
${PATCH_CHECKSUMS[$PATCH_NUM]}
patch -p0 -i $PATCHFILE
done

Though in that case we are downloading upstream patches, since that's how Bash does bugfix releases.


termux_step_post_make_install() {
cd "${TERMUX_PKG_SRCDIR}"
install -Dm755 "target/$CARGO_TARGET_NAME/release/rnote" -t "$TERMUX_PREFIX/bin"
Copy link
Member

Choose a reason for hiding this comment

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

Going by what is included in Arch Linux's rnote package we also want to include:

  • /bin/rnote-cli
  • /share/fonts/rnote-fonts/*
  • /share/glib-2.0/schemas/com.github.flxzt.rnote.gschema.xml
  • /share/icons/hicolor/scalable/apps/com.github.flxzt.rnote.svg
  • /share/icons/hicolor/scalable/mimetypes/application-rnote.svg
  • /share/icons/hicolor/symbolic/apps/com.github.flxzt.rnote-symbolic.svg
  • /share/locale/*/LC_MESSAGES/rnote.mo
    Although this may not apply since Android doesn't have proper locale support
  • /share/metainfo/com.github.flxzt.rnote.metainfo.xml
  • /share/mime/packages/com.github.flxzt.rnote.xml
  • /share/rnote/*
    Mentally prepend $TERMUX_PREFIX to each of those locations

I'll update these with the location they get compiled to later after I do a local build.

Copy link
Member

Choose a reason for hiding this comment

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

Why are we disabling sound?
Unless PulseAudio causes compilation issues please leave it enabled.

If it is causing compilation issues, turning it off for now will be fine,
but having working sound would be a worthwhile enhancement to add later.

gtk_update_icon_cache: true,
update_desktop_database: true,
- update_mime_database: true,
+ update_mime_database: false,
Copy link
Member

Choose a reason for hiding this comment

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

The MIME database defintely works on Termux, so please leave it enabled if it doesn't cause a build failure.

TERMUX_PKG_HOMEPAGE="https://github.com/flxzt/rnote"
TERMUX_PKG_DESCRIPTION="An infinite canvas vector-based drawing application for handwritten notes"
TERMUX_PKG_LICENSE="GPL-3.0"
TERMUX_PKG_MAINTAINER="@termux"
Copy link
Member

Choose a reason for hiding this comment

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

Since there are quite extensive patches included with this package please consider setting yourself as the maintainer.

Suggested change
TERMUX_PKG_MAINTAINER="@termux"
TERMUX_PKG_MAINTAINER="TERMUX_PKG_MAINTAINER="<Name or Pseudonym> @EDLLT"

@TomJo2000
Copy link
Member

Almost forgot the most important thing.

A huge thanks to you for your work and contribution.

Copy link
Member

@TomJo2000 TomJo2000 left a comment

Choose a reason for hiding this comment

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

Missed a couple things on the initial pass.

TERMUX_PKG_BUILD_DEPENDS="libiconv"

__fetch_gettext_rs() {
# This version is relative to gettext-sys provided by the gettext-rs crate
Copy link
Member

Choose a reason for hiding this comment

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

Gonna need tabs here as well.

Suggested change
# This version is relative to gettext-sys provided by the gettext-rs crate
# This version is relative to gettext-sys provided by the gettext-rs crate

}

__patch_gettext_rs() {
# Patch gettext-rs crate to use a newer gettext version because the old one doesn't compile properly with clang
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
# Patch gettext-rs crate to use a newer gettext version because the old one doesn't compile properly with clang
# Patch gettext-rs crate to use a newer gettext version because the old one doesn't compile properly with clang


__patch_gettext_rs() {
# Patch gettext-rs crate to use a newer gettext version because the old one doesn't compile properly with clang
patch -p1 -d "$TERMUX_PKG_SRCDIR/gettext-source" < "$TERMUX_PKG_BUILDER_DIR"/gettext-rs-crate-patch.diff
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
patch -p1 -d "$TERMUX_PKG_SRCDIR/gettext-source" < "$TERMUX_PKG_BUILDER_DIR"/gettext-rs-crate-patch.diff
patch -p1 -d "$TERMUX_PKG_SRCDIR/gettext-source" < "$TERMUX_PKG_BUILDER_DIR"/gettext-rs-crate-patch.diff

}

termux_step_post_make_install() {
cd "${TERMUX_PKG_SRCDIR}"
Copy link
Member

Choose a reason for hiding this comment

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

Instead of cd-ing, use the absolute path.

Suggested change
cd "${TERMUX_PKG_SRCDIR}"
install -Dm755 "${TERMUX_PKG_SRCDIR}/target/$CARGO_TARGET_NAME/release/rnote" -t "$TERMUX_PREFIX/bin"

termux_step_make() {
termux_setup_rust

cd "${TERMUX_PKG_SRCDIR}"
Copy link
Member

@TomJo2000 TomJo2000 Sep 20, 2024

Choose a reason for hiding this comment

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

We can also get rid of this cd by using TERMUX_PKG_BUILD_IN_SRC=true in the control variable above.

nvm I'm stupid we can't do that here.

}

termux_step_make() {
termux_setup_rust
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be moved to termux_step_pre_configure() since it's setup.

Copy link
Member

Choose a reason for hiding this comment

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

The patches should either all be called *.diff or *.patch, so since the other patches are all *.patch, it would be good to rename this one.

Copy link
Member

Choose a reason for hiding this comment

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

Nvm, I see now , forgot that *.patch files get applied automatically.
Disregard my previous comment.

@TomJo2000
Copy link
Member

Alright I have it building locally.
And I've done most of the cleanup I mentioned above.
Will be pushing that shortly.

temporary commit, refactor build script
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.

[Package]: rnote
3 participants