-
Notifications
You must be signed in to change notification settings - Fork 43
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
Integrate with Starbase #422
base: main
Are you sure you want to change the base?
Conversation
544b200
to
e770bb9
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
869ccd5
to
78a5a37
Compare
cb83ac8
to
c923126
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.
This is a very large changeset and difficult to examine in detail. It seems that the majority of changes are auto-generated so I just did a quick pass on those. I didn't find obvious red flags so if this is passing tests it's probably ok, and existing issues can be dealt with in follow-ups. It's a good idea however to check the comments and address them if applicable.
|
||
from craft_application import LifecycleService | ||
from craft_archives import repo | ||
from craft_cli import emit | ||
from craft_parts import Features, Step, callbacks | ||
from craft_parts import Features, LifecycleManager, Step, callbacks |
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.
Ir seems that this can imported only if TYPE_CHECKING
?
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 got:
rockcraft/services/lifecycle.py:35:29: TCH004 Move import `craft_parts.LifecycleManager` out of type-checking block. Import is used for more than type hinting.
# In test env this could be replaced with sh that exec python by the venv | ||
expected_shebang = ("#!/bin/python3", "#!/bin/sh") |
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'm a bit confused here, how the starbase migration affected this test?
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.
The shebang for that python file got changed to a sh
that exec
with extra environment and parameters. I assume tox
did this.
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.
This one is definitely a problem: the #!/bin/python3
shebang should be set by the Python plugin and the purpose of the test is to verify this. #!/bin/sh
is not an acceptable value, we need to look into this more
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.
#!/bin/sh
'''exec' /run/user/1000/tox_tmp/integration-py3.10/pytest-of-ubuntu/pytest-0/test_python_plugin_ubuntu_ubun1/parts/my-part/install/bin/python3 "$0" "$@"
' '''
# -*- coding: utf-8 -*-
import re
import sys
from hello.__main__ import main
if __name__ == '__main__':
sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])
sys.exit(main())
The file become like this if run under tox
.
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.
Right I understand that, but we have to figure out a way around that because as it is the test is not checking what it should be checking
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 don't see any configuration can disable that. We may have to get a separate test for it without run in tox.
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.
Well, I can create a spread test for that, but it is not possible to do in tox.
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.
Added
pyproject.toml
Outdated
"Operating System :: MacOS :: MacOS X", | ||
"Operating System :: POSIX :: Linux", | ||
"Programming Language :: Python :: 3", | ||
"Programming Language :: Python :: 3.10", | ||
] | ||
requires-python = ">=3.10" | ||
dependencies = [ |
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 don't know if I agree with moving the frozen requirements here. The problem is that this ties Rockcraft to transitive dependencies forever, even if we don't need them anymore, because the application needs to freeze all of its requirements.
What was the reasoning to remove the requirements-*.txt files?
# In test env this could be replaced with sh that exec python by the venv | ||
expected_shebang = ("#!/bin/python3", "#!/bin/sh") |
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.
This one is definitely a problem: the #!/bin/python3
shebang should be set by the Python plugin and the purpose of the test is to verify this. #!/bin/sh
is not an acceptable value, we need to look into this more
ba567e1
to
3d3dfef
Compare
3d3dfef
to
20a0ab9
Compare
No description provided.