Skip to content

Conversation

@firewave
Copy link
Collaborator

No description provided.

@firewave
Copy link
Collaborator Author

We need to merge #4036 first.

@firewave firewave marked this pull request as ready for review May 15, 2022 12:41
@firewave
Copy link
Collaborator Author

@danmar This is now ready for review.

Makefile Outdated
HAVE_RULES=no
endif

# Find available Python interpreter
Copy link
Owner

Choose a reason for hiding this comment

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

hmm.. I can envision this could cause problems for somebody who builds cppcheck using mingw and doesn't have python.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's possible. I wasn't sure if there a case where no Python would be available at all. Not having Python would not be very useful though since you cannot use the matchcompiler or run all tests.

I will try if the detection can be put in a target which is run conditionally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was not able to get it to work inside a target without duplicating the code. For the inline detection I used a shorter form though.

I also added some TODOs for potential issues I came across during testing.

Copy link
Owner

Choose a reason for hiding this comment

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

ok 👍

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

python


checkCWEEntries: /tmp/errorlist.xml
./tools/listErrorsWithoutCWE.py -F /tmp/errorlist.xml
$(eval PYTHON_INTERPRETER := $(if $(PYTHON_INTERPRETER),$(PYTHON_INTERPRETER),$(shell which python3)))
Copy link
Owner

Choose a reason for hiding this comment

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

I don't have a very strong opinion but I think we can require python3 for this script. It's not something a normal user will run.

Copy link
Collaborator Author

@firewave firewave May 15, 2022

Choose a reason for hiding this comment

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

As outlined in #3596 I think we can completely drop Python 2.7. We also already switched to the distro versions of Python so the CI coverage also exists. But that is also outlined in the other PR.

Also after this is merged we can hopefully merge #3596 as well and we default to Python 3.x everywhere.

I would give it a release cycle to sink in, adding deprecation warnings to the build at the start of the next dev cycle (2.9) and drop it in the one afterward (2.10).

Copy link
Owner

Choose a reason for hiding this comment

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

but I wonder what the motivation is to set PYTHON_INTERPRETER here. isn't it enough to use the shebang as we used to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can specify the interpreter when running make:

PYTHON_INTERPRETER=/usr/local/bin/python3.11 make

So this is done for consistency. It will mainly be necessary for testing and MinGW on Windows if the interpreter is not in the PATH.

Before this change it was done once globally but only with MATCHCOMPILER=yes. Since that might not be set and we don't want to do it unconditionally I had to replicate this since I was not able to do it in conditional code.

Copy link
Owner

Choose a reason for hiding this comment

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

As far as I know, until now those that used checkCweEntries was satisfied with the shebang path. It's a script that is used very rarelly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It also fails on platforms like Ubuntu 22.04 where python not longer exists by default and the shebang is not changed yet. The other way around it would fail if no Python 3 is installed by default like CentOS after the shebang is changed. So this is a required change to make the CI work.

@danmar danmar merged commit d299d22 into danmar:main May 22, 2022
@firewave firewave deleted the ub branch May 24, 2022 07:40
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