Skip to content

owut: remove comments and empty lines from the built package - #30374

Open
a-gave wants to merge 1 commit into
openwrt:masterfrom
a-gave:owut
Open

owut: remove comments and empty lines from the built package#30374
a-gave wants to merge 1 commit into
openwrt:masterfrom
a-gave:owut

Conversation

@a-gave

@a-gave a-gave commented Aug 25, 2026

Copy link
Copy Markdown

📦 Package Details

Maintainer: @efahl

Description:
This allows to save some bytes, while preserving the scripts's
copyright and license information.

In owut at current ref db17536ec5cbcc6a76a51e65f3d192b6a2184abb
the reduction in the size applied to the source code is (e.g. without
the computed "s/%%VERSION%%/$(PKG_VERSION)-r$(PKG_RELEASE)/"):

from:
74330 /usr/bin/owut
10541 /usr/share/ucode/utils/argparse.uc

to:
58640 /usr/bin/owut
8630 /usr/share/ucode/utils/argparse.uc


🧪 Run Testing Details

  • OpenWrt Version: 25.12.5
  • OpenWrt Target/Subtarget: mediatek/filogic
  • OpenWrt Device: cudy_wr3000s-v1

✅ Formalities

  • [ x ] I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

@a-gave

a-gave commented Aug 25, 2026

Copy link
Copy Markdown
Author

Incremented the PKG_RELEASE to make the Formality Check happy. It could be reset to 1 at next major update, when changing PKG_VERSION/PKG_SOURCE_DATE.

@efahl

efahl commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

I'd been contemplating that for a while, too. My version skipped the first five lines, so as to leave the copyright and license in the minified source. Do we care about that?

sed -e '5,${/^\/\//d}' ...

@a-gave

a-gave commented Aug 25, 2026

Copy link
Copy Markdown
Author

I'd been contemplating that for a while, too. My version skipped the first five lines, so as to leave the copyright and license in the minified source. Do we care about that?

sed -e '5,${/^\/\//d}' ...

This makes a lot of sense to me. Modified in the last commit e542543
Generally speaking I'd really like to see in openwrt a mechanism that by default actually 'minify' the scripts written in scripting languages like ucode or lua to remove all line breaks and make them a single line.
Or indeed two lines with the first that contains a comment with the information about the package name license and copyright, it seems to be quite a common practice.
I remember trying a few things, but the JavaScript minifiers don't work right away with openwrt's ucode

@efahl

efahl commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

I remember trying a few things, but the JavaScript minifiers don't work right away with openwrt's ucode

Yeah, I tried that first, too. If I remember right, they totally mangled the regexes in the code and it just crashed horribly if you tried to run it.

@efahl efahl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 1 new commit.

Nothing here blocks a merge. The PKG_RELEASE bump is the right call (build-logic change with no PKG_VERSION/PKG_SOURCE_DATE movement), the commit message matches the diff, and I confirmed the transformation is safe against the pinned PKG_SOURCE_VERSION: every deleted line in files/owut and files/argparse.uc is a real full-line comment or blank line at top-level lexical state, and neither file has a multi-line backtick template, so no string literal is disturbed.

The inline notes are one robustness question about the hardcoded 5 and two nits.


Generated by Claude Code

Comment thread utils/owut/Makefile Outdated
Comment thread utils/owut/Makefile Outdated
Comment thread utils/owut/Makefile Outdated

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 1 new commit; no new issues found.

git diff e542543..395fd82 is empty — the Makefile hunk is byte-identical to what I reviewed before, and the only change is the amended commit message. It now names the pinned ref and states the numbers are taken before the %%VERSION%% substitution, which resolves the size-mismatch nit from the previous round.

I re-derived the four figures against efahl/owut at db17536: files/owut 74330 → 58640 and files/argparse.uc 10541 → 8630, exactly as the message claims. Both outputs keep lines 1–4, so the title, Copyright (c) and SPDX-License-Identifier lines survive in each file.

All three FormalityCheck runs are green on the head commit, and the two remaining threads from my last review were both answered by @efahl.


Generated by Claude Code

@efahl

efahl commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

@BKPepe or @GeorgeSapkin This is ready to go.

Comment thread utils/owut/Makefile Outdated
@@ -67,6 +67,10 @@ define Package/owut/install

sed -i -e "s/%%VERSION%%/$(PKG_VERSION)-r$(PKG_RELEASE)/" $(1)/usr/bin/owut

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.

Does it make sense to combine this with the new additions into a single expression?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, much nicer, thanks for the idea.

I just built, installed and ran with this, worked fine, source on device as expected.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed a minor typo in a comment 'licence' to 'license' in 39c8f6b

This allows to save some bytes, while preserving the scripts's
copyright and license information.

In owut at current ref db17536ec5cbcc6a76a51e65f3d192b6a2184abb
the reduction in the size applied to the source code is (e.g. without
the computed "s/%%VERSION%%/$(PKG_VERSION)-r$(PKG_RELEASE)/"):

from:
74330 /usr/bin/owut
10541 /usr/share/ucode/utils/argparse.uc

to:
58640 /usr/bin/owut
8630 /usr/share/ucode/utils/argparse.uc

Signed-off-by: Samuele Longhi <[email protected]>
Co-authored-by: Eric Fahlgren <[email protected]>

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 1 new commit; no new issues found.

git diff 395fd82..39c8f6b is confined to the install recipe: the four sed -i invocations collapse into one that carries all three expressions and both file operands (@GeorgeSapkin's suggestion in this thread), plus the licencelicense typo fix and the added # Hardcode the package version/release comment.

The merge is safe on the two points that could have bitten it:

  • Per-file addressing. 5,$ and the implicit line counter must restart for argparse.uc rather than continuing from the end of owut. GNU sed -i implies -s, so they do — confirmed on sed 4.9 with a two-file reproduction.
  • Expression ordering within one cycle. s/%%VERSION%%/…/ now runs on the same pass as the two d commands instead of in a prior pass. The substitution neither creates nor removes lines and cannot turn a line into ^$ or ^[ \t]*// (the replacement is a non-empty version string), so the two forms are equivalent. I ran the old and new pipelines against synthetic files reproducing both preambles — shebang-first and //----ruler-first — with %%VERSION%% in code, in a comment past line 5, and in the first four lines; output was byte-identical, and lines 1–4 (title / Copyright (c) / SPDX-License-Identifier) survive in both files.

$$$$ quoting and the backslash continuations are unchanged in meaning — make strips only the single recipe-prefix tab from each continued line and hands the rest to the shell as one command. PKG_RELEASE:=2 is still correct for a build-logic-only change, the commit message still matches the diff, and the Signed-off-by matches the commit author.

All 14 checks are green on 39c8f6b, including the ten Feeds Package Test Build architectures, which exercise this recipe for real.

The one thread still open is @GeorgeSapkin's, and it is answered by this commit — leaving it to them to close.


Generated by Claude Code

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.

4 participants