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

[bugfix] Include platform to ldlogger.so path #3976

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

bruntib
Copy link
Contributor

@bruntib bruntib commented Aug 10, 2023

A previous improvement in commit db924b0 requires that ldlogger.so file is available in a directory named the current platform. When building a Python package from CodeChecker, this ldlogger.so is not placed in such a directory.
This patch fixes this issue.

@bruntib bruntib added bugfix 🔨 package-Python 🐍 Issues related to the officially supplied PyPI (pip) package labels Aug 10, 2023
@bruntib bruntib added this to the release 6.23.0 milestone Aug 10, 2023
@bruntib bruntib requested a review from vodorok as a code owner August 10, 2023 14:05
setup.py Outdated Show resolved Hide resolved
@whisperity whisperity changed the title [bugfix] Include platform to ldlogger.so path [bugfix] Include platform to ldlogger.so path Aug 23, 2023
A previous improvement in commit db924b0 requires that ldlogger.so
file is available in a directory named the current platform. When
building a Python package from CodeChecker, this ldlogger.so is not
placed in such a directory.
This patch fixes this issue.
@whisperity
Copy link
Member

Generally I'm okay with this. The usual question, however, is whether we could have at least a very simple test? Because supposedly for the failed release, we still tested the fact that logging works, so... how come those tests were passing and yet the release was bogus?

@bruntib
Copy link
Contributor Author

bruntib commented Aug 24, 2023

This bug appeared only in the pypi package, because this package builder script placed the .so file to a different path. We don't have pypi package specific tests. If we want to do that properly then theoretically we should double the current tests so they run in a "pip install"-ed environment too.

@whisperity
Copy link
Member

This bug appeared only in the pypi package, because this package builder script placed the .so file to a different path.

Understood.

We don't have pypi package specific tests. If we want to do that properly then theoretically we should double the current tests so they run in a "pip install"-ed environment too.

Theoretically yes, but I do agree that would be overkill. Nevertheless, just to ensure this does not bite us in the backside down the line at some point, we should have at least a simple test: bake a PyPI-ish package, install it to a local virtualenv, and then inside that env, just log a gcc hello.c and assert that the output file is non-empty?

There is this thing: http://github.com/Ericsson/codechecker/actions/workflows/pypi.yml But this keeps failing and it looks like it's only meant to trigger on releases? — which means we will never know this test case failing before we end up publishing yet another broken release.

# Triggers the workflow on 'release' request events.
# The pypi package will be published only on the release event.
on:
  release:
    types: [published]

@Katze719
Copy link

hey, have the same problem, when do you think the fix will be merged into main?

@bruntib
Copy link
Contributor Author

bruntib commented Sep 25, 2023

@whisperity So what do you suggest? Would it be fine if I replaced the current on section of pypi.yml to on: [pull_request]?
@Katze719 I'll add this small modification to the test environment and hopefully it can be merged after.

@whisperity
Copy link
Member

@whisperity So what do you suggest? Would it be fine if I replaced the current on section of pypi.yml to on: [pull_request]?

@bruntib Given the fact that the PyPI job kept failing for about the past 4 months now, I say we should merge this patch as-is, and have a follow-up patch which fixes the PyPI job and adds the testing necessary to catch mistakes that let this issue surface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix 🔨 package-Python 🐍 Issues related to the officially supplied PyPI (pip) package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants