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

fix(scripts/termux_step_install_license): fix logic error #21005

Merged
merged 2 commits into from
Aug 2, 2024

Conversation

TomJo2000
Copy link
Member

Move counter advance to the end of the while loop.
This fixes a logic error where the counter wouldn't advance if the first license was "generic"
and the second one was author specific.
This would cause cp to attempt to copy through the dangling symlink to the generic license
in termux-licenses which fails and dies.

I also moved the $TERMUX_PACKAGE_LIBRARY check into a case which should make it easier to follow.

@TomJo2000
Copy link
Member Author

I'll do some local searching for any packages effected by this logic error
and add them to this PR with the appropriate TERMUX_PKG_LICENSE_FILE line removed where possible

@TomJo2000 TomJo2000 force-pushed the fix-license-counting branch 2 times, most recently from c5c82d8 to 1fbb7bf Compare July 31, 2024 21:51
@TomJo2000 TomJo2000 force-pushed the fix-license-counting branch 2 times, most recently from caa8021 to 6a94e07 Compare July 31, 2024 23:49
@TomJo2000
Copy link
Member Author

I added the gtkwave changes to this PR temporarily.
I'll be spinning them out into a separate PR later but I wanted to test it against the CI really quick.
And also it's 2am and I wanna work on this on my laptop tomorrow, so pushing it here was convenient.

}
done
if (( ! FROM_SOURCES )); then
# If we get here no license file could be found
termux_error_exit "${TERMUX_PKG_NAME}: Could not find a license file for $LICENSE in the package sources"
Copy link
Member

Choose a reason for hiding this comment

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

Add a comma , after here and in below comment too.

Copy link
Member Author

@TomJo2000 TomJo2000 Aug 1, 2024

Choose a reason for hiding this comment

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

Actually I think explaining the second if (( ! FROM_SOURCES )); then works better here.
See the comment above the if statement that I just added.

Copy link
Member

Choose a reason for hiding this comment

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

You need to move the second if (( ! FROM_SOURCES )); then inside the first one, otherwise when second non-generic license is processed, the second if will get triggered and abort.

And use "If we have not found".

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah you're right it would need to be defined outside the while loop otherwise it gets reset for each license.

Copy link
Member

Choose a reason for hiding this comment

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

the second if will get triggered and abort.

Sorry, it won't abort, but wrong place for it. The second abort if (( ! FROM_SOURCES )) should be inside the first if (( ! FROM_SOURCES )) in which licenses are actually searched so that it isn't run after first non-generic license loop.

And now you have moved the FROM_SOURCES initialization to the wrong if (TERMUX_PKG_LICENSE_FILE).

Copy link
Member

Choose a reason for hiding this comment

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

lolz, true, do it then. I should have caught it earlier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just pushed it in the latest revision

Copy link
Member

Choose a reason for hiding this comment

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

Great! Now let's wait for the next production break

Copy link
Member Author

@TomJo2000 TomJo2000 Aug 2, 2024

Choose a reason for hiding this comment

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

Does not remebering to change the commit message to actually reflect the final state of the PR count?

Copy link
Member

Choose a reason for hiding this comment

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

Wrong commit message is a getting-kicked-out-of-org offense!

@agnostic-apollo
Copy link
Member

Yeah, I am gonna take a break too. Maybe grimler has some input too. Can do this tomorrow.

@TomJo2000 TomJo2000 force-pushed the fix-license-counting branch 7 times, most recently from 7bd90f6 to 2eb17c1 Compare August 1, 2024 07:58
@termux termux deleted a comment from Almoshaks964 Aug 1, 2024
@TomJo2000 TomJo2000 force-pushed the fix-license-counting branch 2 times, most recently from 7fcce67 to 9c28a98 Compare August 2, 2024 17:05
Move counter advance to the end of the while loop.
This fixes a logic error where the counter wouldn't advance
if the first license was "generic", and the second one was
author specific. This would cause `cp` to attempt to copy
through the dangling symlink to the generic license
in `termux-licenses` which fails and dies.
@TomJo2000 TomJo2000 merged commit 377612c into termux:master Aug 2, 2024
5 checks passed
@TomJo2000 TomJo2000 deleted the fix-license-counting branch August 2, 2024 17:30
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.

2 participants