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

Ci add codeql #23812

Merged
merged 3 commits into from
Nov 30, 2022
Merged

Ci add codeql #23812

merged 3 commits into from
Nov 30, 2022

Conversation

tacaswell
Copy link
Member

PR Summary

replaces #22446

@tacaswell tacaswell added this to the v3.7.0 milestone Sep 6, 2022
@tacaswell tacaswell mentioned this pull request Sep 6, 2022
Closed
@tacaswell
Copy link
Member Author

I will squash this down once it is working.

@tacaswell
Copy link
Member Author

I tried manually dismissing all of the warnings from exten/ or build/, lets see if that reduces the noise?

I'm a bit unclear where that state lives though.

oscargus
oscargus approved these changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@oscargus oscargus left a comment

Choose a reason for hiding this comment

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

I think this looks good! Probably we still want someone more experienced than me to say something more insightful... (I am primarily experienced in browsing static code checking results, not setting them up...)

@tacaswell
Copy link
Member Author

I would also like feedback from @QuLogic or @anntzer about the c++ changes to deal with the overflows (I did it two ways, is one better than the other? Is it important to be consistent here? should we have just pushed everything to bigger types?).

@anntzer
Copy link
Contributor

anntzer commented Sep 7, 2022 •
edited

It would probably be better to be consistent and use ssize_t everywhere, but let's not lose sleep over that for now.

Also, AFAICT the whole to_string_argb is completely unused (and can be easily implemented with numpy, it's the similar to one-line byteorder swap in cbook._premultiplied_argb32_to_unmultiplied_rgba8888 without the extra alpha handling), so I would just deprecate it together with the Python-level wrappers.

@tacaswell tacaswell marked this pull request as draft October 20, 2022 22:06
@tacaswell
Copy link
Member Author

Back to draft because the build does not work again...

anntzer
anntzer reviewed Oct 21, 2022
View reviewed changes
src/_backend_agg.h
@@ -1124,7 +1124,7 @@ class QuadMeshGenerator

inline size_t num_paths() const
{
return m_meshWidth * m_meshHeight;
return (size_t) m_meshWidth * m_meshHeight;
Copy link
Contributor

Choose a reason for hiding this comment

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

py_adaptors.h defines num_paths as Py_ssize_t, perhaps better for consistency, though anything will work, of course.

@anntzer
Copy link
Contributor

anntzer commented Oct 21, 2022

Nearly all the changes just hit the now deprecated to_string{,_argb}; just left an additional point re: signedness of num_paths, though it's not a big deal either way.

@QuLogic
Copy link
Member

QuLogic commented Nov 26, 2022

OK, this is now passing, though I will likely want to clean up the YAML. The JavaScript analysis takes ~6m; the C++/Python one takes ~17m. I think I can split C++/Python back up again now that it's working so that it'll finish quicker. Hopefully we don't end up with too many workflows running in parallel.

@QuLogic QuLogic force-pushed the ci_add_codeql branch 4 times, most recently from e3e83db to 7430026 Compare November 26, 2022 06:26
@QuLogic @oscargus @tacaswell
CI: Add CodeQL analysis
41833f5 
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>
tacaswell and others added 2 commits November 26, 2022 02:43
@tacaswell @QuLogic
MNT: ensure pre-multiplication casting to prevent overflow
64fc13a
@QuLogic
BLD: Add an explicit message if certifi is unavailable
340eee2 
If you only do a build step (i.e., `python setup.py build`), it does not
go through all the build dependency machinery, and then mysteriously
says it 'cannot download' a URL, but really it's a missing dependency.
@QuLogic
Copy link
Member

QuLogic commented Nov 26, 2022

I squashed down everything, removed the C++ change and confirmed that CodeQL is still warning about it, so I've now added it back in along with a more explicit message about why the build broke earlier.

@QuLogic QuLogic marked this pull request as ready for review November 26, 2022 08:16
@tacaswell
Copy link
Member Author

This is my PR so I can not approve it, but 👍🏻 from me.

Do we want to standardize the c++ any more or not worry about that right now?

@QuLogic
Copy link
Member

QuLogic commented Nov 30, 2022

I think since they're deprecated, it's fine to leave them slightly inconsistent since they'll be deleted.

@tacaswell tacaswell merged commit 8f0003a into matplotlib:main Nov 30, 2022
@tacaswell tacaswell deleted the ci_add_codeql branch November 30, 2022 21:01
@QuLogic QuLogic mentioned this pull request Jul 21, 2023
Closed
@QuLogic QuLogic added the CI: testing CI configuration and testing label Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anntzer anntzer anntzer left review comments

@oscargus oscargus oscargus approved these changes

Assignees
No one assigned
Labels
CI: testing CI configuration and testing
Projects
None yet
Milestone
v3.7.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tacaswell @anntzer @QuLogic @oscargus