search

Dobiasd / FunctionalPlus

Functional Programming Library for C++. Write concise and readable C++ code.
http://www.editgym.com/fplus-api-search/
Boost Software License 1.0
2.07k stars 168 forks source link

CI: do not require admin rights #305

Closed pthom closed 1 month ago

pthom commented 1 month ago

Hello Dobias.

I hope everything is fine on your side. I tried to address the segfault under windows. I will give you more details and the related issue.

The content of this pull request is different. It is only a patch I did because I wanted to be able to use the CI scripts when not being an administrator. This is important under windows where the standard install would install into C:\Program Files\doctest, which is a path with a space inside something that CMake are cygwin are allergic to.

Dobiasd commented 1 month ago

Thanks!

What do you think about de-duplicating the code block currently in script/ci_build.sh and script/ci_setup_dependencies.sh into something like a new script/determine_install_prefix.sh?

pthom commented 1 month ago

What do you think about de-duplicating the code block currently in script/ci_build.sh and script/ci_setup_dependencies.sh into something like a new script/determine_install_prefix.sh?

I will do it

pthom commented 1 month ago

Okay there is only one script now: https://github.com/Dobiasd/FunctionalPlus/pull/305/files#diff-e458ac2c3066a16d44d83d554bc8ae75b49c4cc647d1e702e6f02eeb68433657

And the CI actions are now simpler: they will call this script only once with the correct argument (run_build or run_tests).

Functionally there is only one change: codeql.yml used to build without -DCMAKE_BUILD_TYPE=Release. It now does.

I cannot access your CodeQL logs. Please check if they are unchanged.

By the way, are you happy with CodeQL? I might consider adding it to some of my projects.

pthom commented 1 month ago

And the CI now passes (the issue was inside GitHub runner, and fixed by GitHub)

Dobiasd commented 1 month ago

And the CI now passes (the issue was inside GitHub runner, and fixed by GitHub)

Indeed. I just re-ran the CI in master and it's green now. :tada:

I cannot access your CodeQL logs. Please check if they are unchanged.

Here they are:

By the way, are you happy with CodeQL? I might consider adding it to some of my projects.

Honestly, I have not interacted with it since it was added. :grimacing: So I can't give a recommendation.

And the CI actions are now simpler: they will call this script only once with the correct argument (run_build or run_tests).

Simpler than at the beginning of this PR, yes. But I'm not sure if they are simpler now here than in master (i.e., without the PR). At least there are 51 more lines than before.

If there had not been this temporary problem with the Windows GitHub Actions runner, would you still favor this PR?

pthom commented 1 month ago

CodeQL: Honestly, I have not interacted with it since it was https://github.com/Dobiasd/FunctionalPlus/pull/226. 😬 So I can't give a recommendation.

As far as I can see on my fork, there is some interesting information in the security tab.

image

Those information are accessible only to the repository owner. I guess this is for security reasons. However, by cloning your repository I can see all the diagnostics: It seems like there is a checkbox where you can look at all the diagnosis and choose to ignore them / some of them.

image

If there had not been this temporary problem with the Windows GitHub Actions runner, would you still favor this PR?

Yes, because when I want to work on your project under Windows, I always have to set up doctest. And it's a manual step, which is always tedious (as opposed to simply running the script). There is no official equivalent to a global library install in Windows (/usr/local/include et. al)

Dobiasd commented 1 month ago

Ok, that makes sense. Then, let's merge. :+1:

Dobiasd commented 1 month ago

And thanks for the CodeQL motivation. I'll look into it. :crossed_fingers: