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

Remove threading compatibility layer #5570

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Conversation

KKoukiou
Copy link
Contributor

@KKoukiou KKoukiou commented Apr 16, 2024

We cannot have a module called 'threading', otherwise the build will fail with
the traceback below. Remove this file as it was anyhow meant to be an
intermediate compatibility solution.

The traceback from the build:

Traceback (most recent call last):
File "", line 2, in
File "/usr/lib64/python3.12/py_compile.py", line 9, in
import importlib.util
File "", line 16, in
File "/builddir/build/BUILD/anaconda-41.10/pyanaconda/threading.py", line 21, in
from pyanaconda.core.threads import thread_manager as threadMgr
ModuleNotFoundError: No module named 'pyanaconda'

Resolves: rhbz#2275279

Blocked by:

@pep8speaks
Copy link

Hello @KKoukiou! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 42:1: E402 module level import not at top of file

Line 36:1: E402 module level import not at top of file

@KKoukiou
Copy link
Contributor Author

/kickstart-test --testtype smoke

@KKoukiou KKoukiou changed the title Rename pyanaconda.modules.common.threads Remove threading compatibility layer Apr 16, 2024
@jkonecny12
Copy link
Member

custom_threads is not a good name. First, it doesn't mean anything. Second, it's too generic that I wouldn't be surprised by another collision in the future.

If we want to do this, I would rather like to see something like anaconda_threads. However, I'm not sure about this solution in overall. Python is namespaced already by pyanaconda package. Issues we see is a tooling issue not language one.

@jkonecny12
Copy link
Member

Also I see only removal of file in this PR, isn't there something missed here?

@M4rtinK
Copy link
Contributor

M4rtinK commented Apr 17, 2024

If we want to do this, I would rather like to see something like anaconda_threads. However, I'm not sure about this solution in overall. Python is namespaced already by pyanaconda package. Issues we see is a tooling issue not language one.

Agreed that if this is about a naming collision, a replacement should have "anaconda" in its name to fix it for good. Provided we really need it.

The suggestion from Miro on the bug to stop using this weird pycompile script also sounds interesting: https://bugzilla.redhat.com/show_bug.cgi?id=2275279#c15

But indeed, hard to tell how hard it it is to change it now & what other unexpected side effects it might have (maybe something depends on the pyc files in a weird way in our CI & might break if they are not there (in built but unpackaged code) ?).

@jkonecny12
Copy link
Member

More prefered solution to this issue: #5574

Thanks a lot @KKoukiou for such a quick reaction ;)

Copy link

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the stale label Jun 17, 2024
@KKoukiou KKoukiou removed the stale label Jun 17, 2024
We cannot have a module called 'threading', otherwise the build will fail with
the traceback below. Remove this file as it was anyhow meant to be an
intermediate compatibility solution.

The traceback from the build:

Traceback (most recent call last):
  File "<string>", line 2, in <module>
  File "/usr/lib64/python3.12/py_compile.py", line 9, in <module>
    import importlib.util
  File "<frozen importlib.util>", line 16, in <module>
  File "/builddir/build/BUILD/anaconda-41.10/pyanaconda/threading.py", line 21, in <module>
    from pyanaconda.core.threads import thread_manager as threadMgr
ModuleNotFoundError: No module named 'pyanaconda'

Resolves: rhbz#2275279
@KKoukiou
Copy link
Contributor Author

@jkonecny12 I think we can merge this now that inital setup released. Checked kdump for possible breackable - seemed not affected. If some other plugin will be affected, it's fine - we will fix it.

@jkonecny12
Copy link
Member

I think we should also propose fix for oscap addon before doing this:
https://github.com/search?q=repo%3AOpenSCAP%2Foscap-anaconda-addon%20threading&type=code

@M4rtinK
Copy link
Contributor

M4rtinK commented Jul 23, 2024

I think we should also propose fix for oscap addon before doing this:
https://github.com/search?q=repo%3AOpenSCAP%2Foscap-anaconda-addon%20threading&type=code

Did we check the Anaconda kdump addon is compatible with the new thing ?
https://github.com/rhinstaller/kdump-anaconda-addon

@jkonecny12
Copy link
Member

Looks kdump should be fine.

For OSCAP @KKoukiou created an issue: OpenSCAP/oscap-anaconda-addon#253

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks good to me now.

@KKoukiou
Copy link
Contributor Author

/kickstart-tests --testtype smoke

@VladimirSlavik
Copy link
Contributor

Thanks for pushing forward with this! It's one of those eternal todos...

@KKoukiou
Copy link
Contributor Author

/kickstart-tests --testtype smoke

@KKoukiou KKoukiou merged commit ffd8065 into rhinstaller:master Jul 25, 2024
15 checks passed
@KKoukiou KKoukiou deleted the threading branch July 25, 2024 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 participants