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

Fix several issues noted in #343 and #347 #353

Merged
merged 4 commits into from
Dec 18, 2024

Conversation

henry2cox
Copy link
Collaborator

No description provided.

@hartwork
Copy link
Contributor

hartwork commented Dec 18, 2024

@henry2cox if #347 is fixed here, we can drop line…

sudo ln -s python3-coverage /usr/bin/coverage # until issue #347 is fixed

…here and have the CI confirm (to some extent) that the fix (is complete and) works as expected.

@henry2cox
Copy link
Collaborator Author

@henry2cox if #347 is fixed here, we can drop line…

ah..right. Oversight.
Will update the version in my sandbox and force-push again (I assume that that is an acceptable use model? Or do we expect something different?)

@hartwork
Copy link
Contributor

hartwork commented Dec 18, 2024

Will update the version in my sandbox and force-push again (I assume that that is an acceptable use model? Or do we expect something different?)

@henry2cox I believe there are two conflicting schools about it: school A (including me) would say the branch is yours and yours only you can force push into it until it's merged and school B (including CPython) would ask to only add new commits on top for all changes you make during review.

@@ -65,7 +65,6 @@ jobs:

sudo perl -MCPAN -e 'install(Memory::Process)' # no package in Ubuntu

Copy link
Contributor

Choose a reason for hiding this comment

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

(just an idea: I think that blank line before could also be dropped)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure...why not?

@hartwork
Copy link
Contributor

@henry2cox the CI likes it 🎉

@@ -62,10 +62,8 @@ jobs:

sudo apt-get update
sudo apt-get install --no-install-recommends --yes -V "${ubuntu_packages[@]}"

Copy link
Contributor

Choose a reason for hiding this comment

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

@henry2cox that's the wrong blank line now. My point was about the two consecutive blank lines below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sigh.

Copy link
Contributor

Choose a reason for hiding this comment

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

@henry2cox looks just right now though 👍

…eir Coverage.py execuable.

Default is retrieved from 'COVERAGE_COMMAND' environment variable, or is set to 'coverage' otherwise.
See linux-test-project#347

Signed-off-by: Henry Cox <[email protected]>
Signed-off-by: Henry Cox <[email protected]>
@hartwork
Copy link
Contributor

@henry2cox my vote for merging but without squash-merge to keep the commit cutting intact on master.

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