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

enhance(main/dart): enable auto updates #21508

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

Conversation

TomJo2000
Copy link
Member

I also took the opportunity to do some miscellaneous build script cleanup,
switching to the GitHub source also allows us to enable auto updates for dart.

PR checklist

  • Builds locally
  • Builds on CI
  • Package content is complete
  • Tested on Device

I would appreciate it if @priyanuj-gogoi could check this over,
reviews from anyone else are of course welcome as always.

and various build script cleanup
@TomJo2000 TomJo2000 force-pushed the dart-source branch 3 times, most recently from e5126bd to 01ac780 Compare September 19, 2024 10:45
@samujjal-gogoi
Copy link

samujjal-gogoi commented Sep 19, 2024

I would appreciate it if @priyanuj-gogoi could check this over

Yes, I'll surely love to do the review!

PS: I'm @priyanuj-gogoi (Using a different account)

Comment on lines +22 to +23
# Get latest release tag:
local api_url="https://api.github.com/repos/dart-lang/sdk/git/refs/tags"

Choose a reason for hiding this comment

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

Use https://storage.googleapis.com/dart-archive/channels/stable/release/latest/VERSION (official) instead of the GitHub API to fetch latest stable dart version.

The response of that GitHub API URL is too large and you've to filter the latest release later in which case it's not ideal.

TERMUX_PKG_SKIP_SRC_EXTRACT=true
TERMUX_PKG_AUTO_UPDATE=true
TERMUX_PKG_UPDATE_TAG_TYPE='newest-tag'
TERMUX_PKG_UPDATE_VERSION_REGEXP='^\d+.\d+.\d+$'

Choose a reason for hiding this comment

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

Although this regex pattern is valid but it's not sed compatible (like \d is not supported, + should be \+).

Prefer [0-9]\+\.[0-9]\+\.[0-9]\+.

Copy link
Member

Choose a reason for hiding this comment

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

That check is done here.

# If needed, filter version numbers using grep regexp.
if [[ -n "${TERMUX_PKG_UPDATE_VERSION_REGEXP:-}" ]]; then
# Extract version numbers.
local OLD_LATEST_VERSION="${LATEST_VERSION}"
LATEST_VERSION="$(grep -oP "${TERMUX_PKG_UPDATE_VERSION_REGEXP}" <<< "${LATEST_VERSION}" || true)"
if [[ -z "${LATEST_VERSION:-}" ]]; then
termux_error_exit <<-EndOfError
ERROR: failed to filter version numbers using regexp '${TERMUX_PKG_UPDATE_VERSION_REGEXP}'.
Ensure that it works correctly with ${OLD_LATEST_VERSION}.
EndOfError
fi
unset OLD_LATEST_VERSION
fi

# Get latest release tag:
local api_url="https://api.github.com/repos/dart-lang/sdk/git/refs/tags"
local latest_refs_tags
latest_refs_tags=$(curl -s "${api_url}" | jq '.[].ref' | sed -ne "s|.*${TERMUX_PKG_UPDATE_VERSION_REGEXP}\"|\1|p")

Choose a reason for hiding this comment

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

I'm facing an error with the sed expression.

sed: -e expression #1, char 23: invalid reference \1 on `s' command's RHS

Perhaps, you've forgotten to add the group construct? i.e

"s|.*\(${TERMUX_PKG_UPDATE_VERSION_REGEXP}\)\"|\1|p"

@samujjal-gogoi
Copy link

samujjal-gogoi commented Sep 19, 2024

Consider removing termux_step_pre_configure() since it's not required now and has been a bloat code for months (see io.dart file, there are no references of requiring any path fixes.)

@samujjal-gogoi
Copy link

samujjal-gogoi commented Sep 19, 2024

So, I've tried testing the build artifact and looks like the build built 3.6.0 instead of the stable release.

You can verify it by yourself:

cat $PREFIX/lib/dart-sdk/version

The fetch dart shouldn't be executed in post source get (since we already cloned the repo).

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.

3 participants