Skip to content

Ports: Add argparse v3.2 and improve run() and curl helpers - #26934

Open
tbhaxor wants to merge 3 commits into
SerenityOS:masterfrom
tbhaxor:feat/ports-argparse
Open

Ports: Add argparse v3.2 and improve run() and curl helpers#26934
tbhaxor wants to merge 3 commits into
SerenityOS:masterfrom
tbhaxor:feat/ports-argparse

Conversation

@tbhaxor

@tbhaxor tbhaxor commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

No description provided.

Define curlopts with --fail, --location, --silent, and --show-error,
and expand it as an array at the download call site. That removes the
extra --fail -L flags from do_download_file and keeps downloads quiet
while still printing errors.

These flags match the common -fsSL shorthand.
Replace the hand-rolled `echo "+ $@"` traces in run() and run_nocd()
with `set -x` inside the existing subshells. Bash then prints the
invoked command with proper quoting.

run_nocd() sets PS4 to `+ (nocd) ` so traces still show the nocd
marker. run() enables xtrace after changing directory so
`cd $workdir` is not printed on every call.
@tbhaxor
tbhaxor requested a review from timschumi as a code owner August 14, 2026 15:33
@github-actions github-actions Bot added the 馃憖 pr-needs-review PR needs review from a maintainer or community member label Aug 14, 2026
Comment thread Ports/.port_include.sh

installedpackagesdb="${DESTDIR}/usr/Ports/installed.db"

curlopts=(--fail --location --silent --show-error)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we want the download to be silent?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's more of to silent the noisy progress which doesn't get live updated. So its better to silent this, mainly error should be shown.

Comment thread Ports/README.md

Defaults to `configure`.

#### `curlopts`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nothing is using this, and I don't think ports should be involved in deciding how exactly their declared files get downloaded.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice point, although port include includes curlopts but it wasn't documented. This changes does that.

If you are still not convinced, I can remove this. Lmk

@linusg linusg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It also seems a bit pointless to add a library port that no other port depends on?

@tbhaxor

tbhaxor commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

It also seems a bit pointless to add a library port that no other port depends on?

I see what you are trying so take. Will remove this in the following fixups. Can you plz re-review the PR in mean time?

@tbhaxor
tbhaxor requested a review from linusg August 15, 2026 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

馃憖 pr-needs-review PR needs review from a maintainer or community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants