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

Backport CodeQl fixes from Oboe #2548

Merged
merged 1 commit into from
Mar 28, 2022

Conversation

ann0see
Copy link
Member

@ann0see ann0see commented Mar 24, 2022

Short description of changes
One possible solution (what this PR does) is to just upgrade Oboe to the commit where the CodeQl fixes were included. That's what this does.

CHANGELOG: Internal: Backport Oboe's fixes CodeQl overflow warnings
Context: Fixes an issue?
Fixes: #2545

Does this change need documentation? What needs to be documented and how?
No.

Status of this Pull Request
Ready for review
What is missing until this pull request can be merged?
Test on Android, verification that the CodeQl errors disappeared.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@hoffie
Copy link
Member

hoffie commented Mar 24, 2022

Was that a cherry-pick of the fix? I don't think this can work. Submodule contents are not stored as part of our git and your cherry-pick is not part of upstream, so it's nowhere to get from.

Our options are:

  • Replace the submodule by a copy of the code (I'd like to avoid that...)
  • Use google/oboe@master
  • Fork google/oboe to our org, use the latest release and cherry-pick the fixes we want. We can then update the submodule to use that fork with the cherry-picked rev.

@ann0see
Copy link
Member Author

ann0see commented Mar 24, 2022

Yes, I used a cherry pick. And the CI failed.
Maybe the forking idea is possible for a short solution.

@ann0see ann0see force-pushed the patch/upgradeOboe branch from eda7c85 to c20048a Compare March 24, 2022 20:38
@pljones
Copy link
Collaborator

pljones commented Mar 24, 2022

Can we check out from a specific commit? That way we get the full tree at that point - it means taking all the changes to that point but none after. That relies on the chosen point being safe, of course.

I'd rather take the opportunity to move to latest on the library and test it doesn't break anything. If it does, we can then think about what to do - but then it's harder.

@ann0see
Copy link
Member Author

ann0see commented Mar 24, 2022

Yes, I basically moved to the commit where the CodeQl changes were fixed. However the CI doesn't think so...

@pljones
Copy link
Collaborator

pljones commented Mar 24, 2022

Last master https://github.com/jamulussoftware/jamulus/runs/5680253483?check_suite_focus=true
oboe at 855ea841a93bf304065e5152909983b1b85ffabb

Yours https://github.com/jamulussoftware/jamulus/runs/5682859842?check_suite_focus=true
oboe at 2d4797ad433aa220499d7ea55f4d15a667decc27

What did you want?

@ann0see
Copy link
Member Author

ann0see commented Mar 25, 2022

I want to update to 2d4797ad433aa220499d7ea55f4d15a667decc27

CodeQl however doesn’t seem to detect less errors

@pljones
Copy link
Collaborator

pljones commented Mar 25, 2022

The CodeQL run isn't showing errors. What am I not understanding?

@ann0see
Copy link
Member Author

ann0see commented Mar 25, 2022

Yes. But they should be shown as fixed. Maybe this will show up as soon as this PR is merged?

@pljones
Copy link
Collaborator

pljones commented Mar 25, 2022

It's only showing the "Won't fix" errors, in fact... Either it should have shown the fixes or shown them still being there... weird.

@ann0see
Copy link
Member Author

ann0see commented Mar 26, 2022

Probably merge it and look what happens on new PRs

@hoffie hoffie added this to the Release 3.9.0 milestone Mar 28, 2022
Copy link
Member

@hoffie hoffie left a comment

Choose a reason for hiding this comment

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

Tested: Submodule committish is as expected.
I did not run any further tests on Android for now.

@ann0see ann0see merged commit 178de74 into jamulussoftware:master Mar 28, 2022
@ann0see ann0see deleted the patch/upgradeOboe branch March 28, 2022 09:32
@ann0see
Copy link
Member Author

ann0see commented Mar 28, 2022

Hopefully the CodeQL errors are gone…

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.

Upgrade Oboe to latest commit to fix CodeQL errors
3 participants