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

Project: Refactor build script #49

Merged
merged 7 commits into from
Mar 5, 2025
Merged

Conversation

Luphaestus
Copy link
Contributor

@Luphaestus Luphaestus commented Mar 4, 2025

Removed repeated make commands.
Made help message dynamic and prettier.
Integrated 'lk3rd-build-all.sh'
Added debug flag to more easily show the progress of "all".
Converted User argument to flags.
Allowed the user to run build.sh from any directory.

@BotchedRPR
Copy link
Contributor

Hi, thanks!

First of all - please squash the two commits. We don't do it from our side (because of PRs that are more complex)

Second of all maybe for @halal-beef - shall we make user builds actually... not print debug logs instead of constantly reversing the commit? Nit not for this PR but as an overall idea

Complex review later the day.

Thanks for the work!

@halal-beef
Copy link
Collaborator

Wouldn't this break the build testing CI? I'm pretty sure that relies on our build all script

@halal-beef
Copy link
Collaborator

Second of all maybe for @halal-beef - shall we make user builds actually... not print debug logs instead of constantly reversing the commit? Nit not for this PR but as an overall idea

Sure.

@halal-beef
Copy link
Collaborator

Commit message note btw, please sign off if possible and fix spelling errors, like redundent -> redundant, also please capitalise too.

@halal-beef
Copy link
Collaborator

Commit title note, please follow the linux style commit format, for example here would be

Project: Refactor build script

@halal-beef
Copy link
Collaborator

Wouldn't this break the build testing CI? I'm pretty sure that relies on our build all script

Yep, CI build failed.

@Luphaestus Luphaestus changed the title More complicated build script Project: Refactor build script Mar 4, 2025
@Luphaestus Luphaestus force-pushed the dev branch 4 times, most recently from 53ed95d to edcdf98 Compare March 4, 2025 20:58
@halal-beef
Copy link
Collaborator

CI passed, will do builds on my personal system and check before approving, great work tho.

@BotchedRPR
Copy link
Contributor

nit: please split your commits

nit 2: fix your git email :) you don't get authorship

@halal-beef
Copy link
Collaborator

nit: please split your commits

nit 2: fix your git email :) you don't get authorship

I guess the first nit is splitting adding configurable printing support and the actual build system refactor?

@BotchedRPR
Copy link
Contributor

nit: please split your commits
nit 2: fix your git email :) you don't get authorship

I guess the first nit is splitting adding configurable printing support and the actual build system refactor?

it can stay in one PR, we're not the linux kernel, maybe change the pr name but other than that seems fine to me

remove redundant printf
@Luphaestus
Copy link
Contributor Author

I think I’ve fixed everything? Thank you both for your patience.

@BotchedRPR
Copy link
Contributor

whoops didn't mean to do that, github review on mobile isn't the best

if @halal-beef can confirm that it's functional and the flag change will be added we can merge it in

rearranged config output.
@halal-beef
Copy link
Collaborator

whoops didn't mean to do that, github review on mobile isn't the best

if @halal-beef can confirm that it's functional and the flag change will be added we can merge it in

right, will do

@halal-beef
Copy link
Collaborator

Invalid parameter for --debug. Use 'y' or 'n'.

slight nitpick,

-d -debug debug mode.

to

-d -debug [y/n] debug mode.

@halal-beef
Copy link
Collaborator

oh wait, no sorry but yeah the wording is wrong on that error, should be --verbose

@halal-beef
Copy link
Collaborator

All of the command arguments work, one more nitpick tho in debug.c, please also guard the uart output under the debugging ifdef, otherwise boot may be considerably slower for users

@halal-beef
Copy link
Collaborator

otherwise, amazing work

@halal-beef
Copy link
Collaborator

also one more thing, please do not apply tabs to ifdef

@halal-beef
Copy link
Collaborator

also one more thing, please do not apply tabs to ifdef

image

do this instead

@Luphaestus
Copy link
Contributor Author

oh wait, no sorry but yeah the wording is wrong on that error, should be --verbose

What is your opinion on #49 (comment)

@Luphaestus
Copy link
Contributor Author

otherwise, amazing work

Thanks!

@halal-beef
Copy link
Collaborator

oh wait, no sorry but yeah the wording is wrong on that error, should be --verbose

What is your opinion on #49 (comment)

just gave my comment

@Luphaestus
Copy link
Contributor Author

This commit (4b25823) aims to prevent false-positive CI test results, such as this one, which occurs when bad input parameters are provided.

@halal-beef halal-beef self-requested a review March 5, 2025 04:55
@halal-beef
Copy link
Collaborator

@BotchedRPR any objections? Will merge in a bit, for now I'm extra tired and will call it a day

@BotchedRPR
Copy link
Contributor

@BotchedRPR any objections? Will merge in a bit, for now I'm extra tired and will call it a day

nope, all good

Rename config output to match new flag names
@halal-beef halal-beef merged commit deae548 into exynos990-mainline:dev Mar 5, 2025
1 check passed
@halal-beef
Copy link
Collaborator

Thank you!

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