-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Ci add codeql #23812
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
Ci add codeql #23812
Conversation
I will squash this down once it is working.
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.
49f173d to
c49f75b
Compare
There was a problem hiding this 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...)
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.
a202190 to
19daeff
Compare
Back to draft because the build does not work again...
There was a problem hiding this comment.
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.
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.
28d323b to
9de072b
Compare
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.
e3e83db to
7430026
Compare
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com> Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>
7430026 to
41833f5
Compare
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.
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.
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?
I think since they're deprecated, it's fine to leave them slightly inconsistent since they'll be deleted.
PR Summary
replaces #22446