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

New attempt at wraplock #13854

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

sp1ritCS
Copy link
Contributor

@sp1ritCS sp1ritCS commented Nov 1, 2024

supersedes #13784

mesonbuild/utils/posix.py Fixed Show fixed Hide fixed
mesonbuild/utils/posix.py Fixed Show fixed Hide fixed
mesonbuild/utils/win32.py Fixed Show fixed Hide fixed
mesonbuild/utils/win32.py Fixed Show fixed Hide fixed
@sp1ritCS
Copy link
Contributor Author

sp1ritCS commented Nov 1, 2024

@eli-schwartz Am I correct in my assumption that are those CodeQL results are false positives? Especially since the previous BuildDirLock did the same, I wouldn't know what I've done wrong

@dcbaker
Copy link
Member

dcbaker commented Nov 1, 2024

Those CodeQL alerts do look weird, I wonder if it's getting confused by TYPE_CHECKING imports?

Oh, I see what it's complaining about. Because the DirectoryLock is being exported, and then imported using an as statement, it's pointing out that there are two definitions of DirectoryLock in the same package. I think you could just resolve this by renaming the DirectoryLock in platform to DirectoryLockBase, and then in the !unix and !windows case doing an from .platform import DirectoryLockBase as DirectoryLock

DirectoryLock provides a generic locking implementation the replaces the
previously used BuildDirLock.
To avoid raceconditions, where one instance of meson currently downloads
a subproject defined in a wrapfile, while another either
  a. starts the download itself too
  b. attemts to evaluate the partially downloaded subproject
wraplock introduces a lockfile, which should prevent simultaneous access
of subprojects by wrap between different instances of meson.
@@ -10,23 +10,38 @@
import typing as T

from .core import MesonException
from .platform import BuildDirLock as BuildDirLockBase
from .platform import DirectoryLockBase, DirectoryLockAction

Check failure

Code scanning / CodeQL

Module-level cyclic import Error

'DirectoryLockBase' may not be defined if module
mesonbuild.utils.platform
is imported before module
mesonbuild.utils.posix
, as the
definition
of DirectoryLockBase occurs after the cyclic
import
of mesonbuild.utils.posix.
@@ -10,23 +10,38 @@
import typing as T

from .core import MesonException
from .platform import BuildDirLock as BuildDirLockBase
from .platform import DirectoryLockBase, DirectoryLockAction

Check failure

Code scanning / CodeQL

Module-level cyclic import Error

'DirectoryLockAction' may not be defined if module
mesonbuild.utils.platform
is imported before module
mesonbuild.utils.posix
, as the
definition
of DirectoryLockAction occurs after the cyclic
import
of mesonbuild.utils.posix.
@@ -10,20 +10,35 @@
import typing as T

from .core import MesonException
from .platform import BuildDirLock as BuildDirLockBase
from .platform import DirectoryLockBase, DirectoryLockAction

Check failure

Code scanning / CodeQL

Module-level cyclic import Error

'DirectoryLockBase' may not be defined if module
mesonbuild.utils.platform
is imported before module
mesonbuild.utils.win32
, as the
definition
of DirectoryLockBase occurs after the cyclic
import
of mesonbuild.utils.win32.
@@ -10,20 +10,35 @@
import typing as T

from .core import MesonException
from .platform import BuildDirLock as BuildDirLockBase
from .platform import DirectoryLockBase, DirectoryLockAction

Check failure

Code scanning / CodeQL

Module-level cyclic import Error

'DirectoryLockAction' may not be defined if module
mesonbuild.utils.platform
is imported before module
mesonbuild.utils.win32
, as the
definition
of DirectoryLockAction occurs after the cyclic
import
of mesonbuild.utils.win32.
@sp1ritCS
Copy link
Contributor Author

sp1ritCS commented Nov 1, 2024

@dcbaker it seems like the issue is that platform.py imports mlog, which sometimes imports mesonlib. Not sure what could be done about that

@dcbaker
Copy link
Member

dcbaker commented Nov 1, 2024

It really only needs things from utils.universal. replacing from mesonlib import ... with from .utils.universal import ... maybe?

@eli-schwartz
Copy link
Member

The point of these delayed function-local imports was precisely to ensure that these cycles do not, will not, and cannot exist. Poking at utils.universal directly seems weird. :p Why is CodeQL complaining about this?

@eli-schwartz
Copy link
Member

eli-schwartz commented Nov 1, 2024

And indeed, that commit didn't fix it. It must be complaining about something else.

@sp1ritCS
Copy link
Contributor Author

sp1ritCS commented Nov 1, 2024

And indeed, that commit didn't fix it. It must be complaining about something else.

dropped. Does CodeQL only complain about "newly" introduced issues? Im still confused as to why BuildDirLock did not have that issue given that the imports entirely match

@eli-schwartz
Copy link
Member

You will sometimes find when refactoring code that it detects something it doesn't like and thinks it is brand new issues, possibly combined with fixing an old issue ("the issues must be different! The name changed!").

@sp1ritCS
Copy link
Contributor Author

sp1ritCS commented Nov 1, 2024

Yeah, I just want to check that this could be merged and it wont complain about it for every single new PR like what'd happen if a normal test case fails

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.

3 participants