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

Swap parameter order to match #2327 #2329

Merged
merged 1 commit into from
May 31, 2021

Conversation

DanielaE
Copy link
Contributor

My additional tests (not yet published) for non-char overloads didn't need action 😁

@alexezeder
Copy link
Contributor

alexezeder commented May 30, 2021

Can we run this test using Docker? Because tests that do not run on CI are bad tests, IMHO.

It should be pretty simple to create a Docker image for MSVC and use it on GitHub Actions, but I'm not sure how to install these preview versions. Also, I'm not sure about that license/EULA stuff, but probably it won't be that bad since they have a post about creating containers.

Regarding the performance of this approach, I can say only that performance is the same (or maybe almost the same) as it's running in Github Actions containers, and pulling times for the images are pretty good. So, in terms of time that will be spent on tests in Github Actions, this change won't be too different from adding a new non-Docker config into the workflows/windows.yml file.

@DanielaE
Copy link
Contributor Author

I'm not sure if I understand you correctly. This has nothing to do with 'preview versions'. As soon as GitHub Pipelines switches from msvc 16.9 to 16.10, the test will be executed (and expose problems like #2324). At present, C++20 tests on Windows are still done with compiler version 19.28.29915 a.k.a. 16.9 Update 6 and cmake reports "Module support is disabled.".

What might be really interesting is having a test using a current gcc from the modules branch. From a quick test on CE I could see that this gcc version no longer crashes when compiling the Module Interface Unit. It will need workarounds like msvc 16.10 and later does. I'm a Linux noob and have absolutely no clue how to make this happen. Nathan Sidwell is at Facebook, may be he can provide a test environment.

@alexezeder
Copy link
Contributor

alexezeder commented May 31, 2021

If there is a version of MSVC that is not a preview, supports modules, and can be installed in Docker image, then I still think that while GitHub Actions use 16.9 version, we can use 16.10 by creating a special image. Let's say we will just make available such configs that GitHub Actions don't currently provide. 😉
Same with GCC, why not wrap modules branch in Docker image and use it on CI. 🤔

@DanielaE
Copy link
Contributor Author

Sure, but we need to find someone capable and willing of doing so - in particular maintaining an image with an up-to-date binary of gcc-modules.

@vitaut vitaut merged commit ba4c7f1 into fmtlib:master May 31, 2021
@vitaut
Copy link
Contributor

vitaut commented May 31, 2021

Thank you

@vitaut
Copy link
Contributor

vitaut commented May 31, 2021

As for making sure this test actually compiles, a simple solution that will catch most issues would be to conditionally compile it with the part that uses modules (mostly import) disabled.

@alexezeder
Copy link
Contributor

The better one is to use Docker images on CI. 🙂

I can prepare an auto-maintainable Docker image for GCC + modules if there is nothing on Docker Hub or GitHub Packages. Same for MSVC if their instructions are correct and Build Tools is able to install desired MSVC version. It should be not so hard...

@DanielaE
Copy link
Contributor Author

As I can see, the windows image is updated to msvc 16.10 now and module-test is engaged. 🎉
Something along the lines of #2324 needs to be applied. The macro FMT_HIDE_MODULE_BUGS must be defined and macro FMT_USE_NONTYPE_TEMPLATE_PARAMETERS must be defined to 0 because this code section is newly engaged with 16.10 and there's no solution yet to make this available through the module interface.

I can create a PR with this macro stuff and one of the workarounds for gcc-module.

@vitaut
Copy link
Contributor

vitaut commented May 31, 2021

Please note that I temporarily disabled module-test because it's breaking the build with the update.

@DanielaE
Copy link
Contributor Author

Yeah, I've seen that. Already working on the PR.

@DanielaE DanielaE deleted the fix/argument-order-in-module-test branch May 31, 2021 17:16
@alexezeder
Copy link
Contributor

After some time battling with Docker, I can provide you the gcc-modules Docker image on GitHub Container Registry (also known as ghcr.io). GitHub Actions is used to build this image, furthermore, this workflow is scheduled to be executed every Sunday so that each week Actions will update the image with the latest state of the c++-modules branch.

Since there is no usage of containers in {fmt}, I also prepare a branch (with one commit, actually) in my fork, where I used this Docker image. It forced me to make some minor changes in the CI config, but nothing criminal. It runs successfully, but not so fast as I expected, actually it's 2x times slower 😩. Maybe it's a Docker problem, but all my tests with Docker show that it doesn't slow down execution so much. Maybe it's a GCC problem because it's a development branch. Anyway, I will probably create another image with the released version of GCC to check that.

@DanielaE I hope this is the test environment you asked for 😉

@vitaut The last change I made was to make sure that the Actions container and my Docker container use the same thread count, and they do. But this change shows that just by passing -- -j`nproc` to CMake build command, I was able to reduce build times - one thread vs. two threads. And this works not only for Linux containers but for all of them - About GitHub-hosted runners. So the question is: is the build process on CI single-threaded on purpose?

@DanielaE
Copy link
Contributor Author

DanielaE commented Jun 2, 2021

Awesome - this is marvellous! 🎉
Unfortunately I'm totally busy with my job duties so I cannot try this immediately. But I'm really looking forward to give this a spin!

@vitaut
Copy link
Contributor

vitaut commented Jun 2, 2021

is the build process on CI single-threaded on purpose?

It is not, as usual PRs are welcome =).

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