Test using setup-python directly for common packages #190

lefticus · 2022-02-05T17:21:17Z

gcovr, conan, ninja, cmake are all fully supported with pip install on all 3 platforms
using setup-python and pip3 directly removes one variable from the build process
using pip3 directly to install all 3 provides consistency across platforms

* gcovr, conan, pip, cmake are all fully supported with pip install on all 3 platforms * using setup-python and pip3 directly removes one variable from the build process * using pip3 directly to install all 3 provides consistency across platforms

codecov-commenter · 2022-02-05T17:25:34Z

Codecov Report

Merging #190 (b1f45dc) into main (26a39b1) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main      #190   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           20        20           
=========================================
  Hits            20        20

Flag	Coverage Δ
Linux	`∅ <ø> (∅)`
Windows	`100.00% <ø> (ø)`
macOS	`∅ <ø> (∅)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26a39b1...b1f45dc. Read the comment docs.

lefticus · 2022-02-05T17:32:23Z

This partially a workaround for aminya/setup-cpp#27 but also I think it is not a bad idea, as mentioned in the commit message, it gives consistency across all platforms for the versions and mechanism that common tools are installed.

ddalcino · 2022-02-05T18:09:29Z

Just for clarification: Is this meant to be permanent, or just a temporary fix until aminya/setup-cpp#27 is fixed?

lefticus · 2022-02-05T18:17:47Z

Just for clarification: Is this meant to be permanent, or just a temporary fix until aminya/setup-cpp#27 is fixed?

In my opinion, it would be permanent. I see it as one less potential issue while maintaining this project, and for our users in the future. I don't see any reason to use a mix of chocolatey, package managers, etc, when python gives us the same versions of the same packages across all platforms.

aminya

It is not setup-cpp's fault that the pre-installed Python broke on Windows images. In fact setup-cpp does not call "setup-python" until it actually needed. I do not approve of this, as this is like discarding all the things it was considered in setup-cpp and trying to re-implement them in bash.

aminya · 2022-02-05T19:38:23Z

.github/workflows/ci.yml

+          cmake: false
+          ninja: false


Cmake and Ninja do not need python. Why should they be installed with pip?

Because it gives a simple consistent command that works on all 3 platforms that are supported on github. Since we are working specifically on a github workflow here.

aminya · 2022-02-05T19:39:18Z

.github/workflows/ci.yml

@@ -101,22 +101,26 @@ jobs:
          restore-keys: |
            ${{ runner.os }}-${{ matrix.compiler }}-${{matrix.build_type}}-${{matrix.generator}}-${{matrix.developer_mode}}

+      - uses: actions/setup-python@v2
+
+      - run: pip3 install cmake ninja conan gcovr


This is incomplete. It does not put the paths that conan and gcovr are called from on the path. Have you tried this locally?

https://github.com/aminya/setup-cpp/blob/94827f017e0e4445837e4cc099daf7ce849a059c/src/utils/setup/setupPipPack.ts#L35-L56

It is not setup-cpp's fault that the pre-installed Python broke on Windows images. In fact setup-cpp does not call "setup-python" until it actually needed. I do not approve of this, as this is like discarding all the things it was considered in setup-cpp and trying to re-implement them in bash.

It is not discarding everything setup-cpp is used for. Setup-cpp still does all of the hard work of finding and installing compilers across compilers.

Here I specifically meant the pip installations. As I mentioned in the above comments, it is needed to check if:

* python paths are added to PATH on all platforms * if pip is installed and is up to date

It's good that setup-cpp is now fixed, but I think it's worth mentioning that all of this worked without any other path manipulations. All tests passed and even properly posted coverage reports.

I think that what @aminya’s not saying here is that setup-cpp works just as well on a self-hosted runner. The setup-Python action takes a little more work for that. Some users depend on being able to build their projects with a self-hosted runner; maybe we should try to support them.

We could add additional code here to make self-hosted runners work; there are directions on how to do that here: https://github.com/actions/setup-python.

Or, we could just use setup-cpp.

aminya · 2022-02-05T19:40:53Z

.github/workflows/ci.yml

@@ -101,22 +101,26 @@ jobs:
          restore-keys: |
            ${{ runner.os }}-${{ matrix.compiler }}-${{matrix.build_type}}-${{matrix.generator}}-${{matrix.developer_mode}}

+      - uses: actions/setup-python@v2
+
+      - run: pip3 install cmake ninja conan gcovr


On the GitHub images, it happens that pip3 is on the path. But a lot of the time, it is actually called pip, which means it is needed to check if pip has version 3 or above.

https://github.com/aminya/setup-cpp/blob/94827f017e0e4445837e4cc099daf7ce849a059c/src/utils/setup/setupPipPack.ts#L19-L29

On the GitHub images, it happens that pip3 is on the path. But a lot of the time, it is actually called pip, which means it is needed to check if pip has version 3 or above.

But isn't this yml file only used on github images?

It's specifically in .github/workflows/ci.yml, so shouldn't that be the primary concern here?

lefticus · 2022-02-05T19:59:24Z

It is not setup-cpp's fault that the pre-installed Python broke on Windows images. In fact setup-cpp does not call "setup-python" until it actually needed. I do not approve of this, as this is like discarding all the things it was considered in setup-cpp and trying to re-implement them in bash.

It is not discarding everything setup-cpp is used for. Setup-cpp still does all of the hard work of finding and installing compilers across compilers.

aminya

The issue is fixed in setup-cpp. The pre-installed pip executable is broken on Windows images. The workaround is to use python -m pip install package

aminya/setup-cpp#27 (comment)

I don't think this is needed anymore
https://github.com/cpp-best-practices/cpp_starter_project/actions/runs/1800095489

aminya · 2022-02-05T20:32:32Z

It is not setup-cpp's fault that the pre-installed Python broke on Windows images. In fact setup-cpp does not call "setup-python" until it actually needed. I do not approve of this, as this is like discarding all the things it was considered in setup-cpp and trying to re-implement them in bash.

It is not discarding everything setup-cpp is used for. Setup-cpp still does all of the hard work of finding and installing compilers across compilers.

Here I specifically meant the pip installations. As I mentioned in the above comments, it is needed to check if:

python paths are added to PATH on all platforms
if pip is installed and is up to date

lefticus · 2022-02-06T15:47:11Z

I'm just going to close and remove this PR, since setup-cpp's python integration is working again.

aminya · 2022-02-06T18:54:17Z

Worth mentioning that the pip issue is a serious problem in their launchers. It is affecting many people on Windows.
See the progress here:
pypa/pip#10875

lefticus marked this pull request as ready for review February 5, 2022 17:38

lefticus requested review from aminya and ddalcino February 5, 2022 17:38

ddalcino approved these changes Feb 5, 2022

View reviewed changes

aminya suggested changes Feb 5, 2022

View reviewed changes

lefticus closed this Feb 6, 2022

aminya deleted the use_python_for_common_dependencies branch February 6, 2022 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test using setup-python directly for common packages #190

Test using setup-python directly for common packages #190

lefticus commented Feb 5, 2022 •

edited

codecov-commenter commented Feb 5, 2022 •

edited

lefticus commented Feb 5, 2022

ddalcino commented Feb 5, 2022

lefticus commented Feb 5, 2022

aminya left a comment

aminya Feb 5, 2022 •

edited

lefticus Feb 5, 2022

aminya Feb 5, 2022 •

edited

lefticus Feb 5, 2022 •

edited

ddalcino Feb 5, 2022

ddalcino Feb 5, 2022

aminya Feb 5, 2022 •

edited

lefticus Feb 5, 2022

lefticus commented Feb 5, 2022

aminya left a comment •

edited

aminya commented Feb 5, 2022 •

edited

lefticus commented Feb 6, 2022

aminya commented Feb 6, 2022

Test using setup-python directly for common packages #190

Test using setup-python directly for common packages #190

Conversation

lefticus commented Feb 5, 2022 • edited

codecov-commenter commented Feb 5, 2022 • edited

Codecov Report

lefticus commented Feb 5, 2022

ddalcino commented Feb 5, 2022

lefticus commented Feb 5, 2022

aminya left a comment

Choose a reason for hiding this comment

aminya Feb 5, 2022 • edited

Choose a reason for hiding this comment

lefticus Feb 5, 2022

Choose a reason for hiding this comment

aminya Feb 5, 2022 • edited

Choose a reason for hiding this comment

lefticus Feb 5, 2022 • edited

Choose a reason for hiding this comment

ddalcino Feb 5, 2022

Choose a reason for hiding this comment

ddalcino Feb 5, 2022

Choose a reason for hiding this comment

aminya Feb 5, 2022 • edited

Choose a reason for hiding this comment

lefticus Feb 5, 2022

Choose a reason for hiding this comment

lefticus commented Feb 5, 2022

aminya left a comment • edited

Choose a reason for hiding this comment

aminya commented Feb 5, 2022 • edited

lefticus commented Feb 6, 2022

aminya commented Feb 6, 2022

lefticus commented Feb 5, 2022 •

edited

codecov-commenter commented Feb 5, 2022 •

edited

aminya Feb 5, 2022 •

edited

aminya Feb 5, 2022 •

edited

lefticus Feb 5, 2022 •

edited

aminya Feb 5, 2022 •

edited

aminya left a comment •

edited

aminya commented Feb 5, 2022 •

edited