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.

Sign up for GitHub

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

Jump to bottom

Added 3 changes: #29

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Added 3 changes: #29

wants to merge 1 commit into from

Conversation

crispyoz
Copy link
Contributor

  1. In profile PACKAGE_REPOS env allows you to add new repositories. But running onion_buildenv update_sdk overwrite and changes made to repositories.conf with the openwrt defaults. Added - prefix to any feed in PACKAGE_REPOS, will filter out those feeds when running onion_buildenv update_sdk

  2. Added env DATE_VERSION to profile, if set to "n" the date will no longer be appended to the filename of the generated firmware file

  3. Deleting files from the "additions" directory tree were not reflected when building. Added new function clean_additions() to remove all additions when running update_imagebuilder

@crispyoz
Added 3 changes:
23fded9 
1) In profile PACKAGE_REPOS env allows you to add new repositories. But running onion_buildenv update_sdk overwrite and changes made to repositories.conf with the openwrt defaults.
Added - prefix to any feed in PACKAGE_REPOS, will filter out those feeds when running onion_buildenv update_sdk

2) Added env DATE_VERSION to profile, if set to "n" the date will no longer be appended to the filename of the generated firmware file

3) Deleting files from the "additions" directory tree were not reflected when building. Added new function clean_additions() to remove all additions when running update_imagebuilder
@crispyoz
Copy link
Contributor Author

Consolidated the 3 previous pull request into a single PR

@crispyoz crispyoz mentioned this pull request Feb 14, 2025
Open
@greenbreakfast
Copy link
Contributor

@crispyoz in general, we prefer to keep each PR to a single change. And it looks like your own IMAGE_BUILDER_PACKAGES selections made it into the PR.

I suggest making issues to discuss these items one by one.

Here are my initial comments on the changes:

  1. Yes, we consider the openwrt-imagebuilder directory to be output. Any changes made there will not be carried over. But why is it an issue if the imagebuilder has the openwrt repos configured? As long as no packages from those repos are not included in the firmware there should be no issues.
  2. I would prefer not changing the profile and instead adding a "no date" switch to the build script. Keeping the profile simple makes it easier to use imo
  3. You make a good point. Let's make a new issue and discuss further

@crispyoz
Copy link
Contributor Author

@greenbreakfast

  1. The reason for removing the feeds is two-fold but the main reason is because update_sdk downloads the package details for the feeds, for example luci, which takes so much longer. For you CI it is not much of an issue but when working on fixing a code issue it makes a big difference is terms of time.

  2. I added it to the profile because I thought it may be something many users might want if they have downstream scripts that rely on consistent naming. The current behaviour is not consistent with how the standard build system has named the output files. I'm not a fan of too many command line switches.

  3. It's clearly a defect. Add a file into additions, build, the file is included, remove the file from additions, build, the file is still included. Clearly if the user removes the file from additions it should not be included in the new build.

@greenbreakfast
Copy link
Contributor

@crispyoz

Item 1: removing feeds

think having extra feeds adds a lot of time when using the SDK, but it shouldn't add a lot of time with the image builder. While the image builder does read the contents of the listed repos, this operation should be fast. From my terminal output it looks like most of the time is spent downloading the packages that need to be installed.

On an AWS EC2 instance, it takes just over 1 minute to do both the setup and build steps:

ubuntu@ip-172-31-90-20:~/openwrt-imagebuilder-wrapper$ time bash onion_buildenv setup_imagebuilder
> Downloading compressed Image Builder
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 66.2M  100 66.2M    0     0  21.7M      0  0:00:03  0:00:03 --:--:-- 21.7M
> Verifying download
> Extracting compressed Image Builder
> Updating Image Builder Package Repos
> Preparing Image Builder

real    0m13.848s
user    0m10.131s
sys     0m2.492s

ubuntu@ip-172-31-90-20:~/openwrt-imagebuilder-wrapper$ time bash onion_buildenv build_all_firmware
> Building all firmware images
> Building onion_omega2 firmware image
make: Entering directory '/home/ubuntu/openwrt-imagebuilder-wrapper/openwrt-imagebuilder'
...
make: Leaving directory '/home/ubuntu/openwrt-imagebuilder-wrapper/openwrt-imagebuilder'
> Compiled firmware at /home/ubuntu/openwrt-imagebuilder-wrapper/output

real    0m47.607s
user    0m39.126s
sys     0m11.706s

This minus repo feature should definitely be added to the SDK though! Updating repos for the SDK involves pulling a bunch of code repos. It would save a ton of time to remove some repos

Item 2: date in firmware image filenames

I see what you're saying. I'd still like to keep the profile for the onion firmware simple.
How about: by default the filename includes the date. But if the profile has a DATE_VERSION=n then the datecode is removed from the filenames?

Item 3: cleaning files dir

I think your solution is solid. Can you please make a PR with just that change?

@crispyoz
Copy link
Contributor Author

crispyoz commented Feb 19, 2025 via email

@crispyoz
Copy link
Contributor Author

crispyoz commented Feb 19, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@crispyoz @greenbreakfast