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

CI: do not require admin rights #305

Merged
merged 1 commit into from
Jun 12, 2024
Merged

Conversation

pthom
Copy link
Contributor

@pthom pthom commented Jun 11, 2024

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
Copy link
Owner

Dobiasd commented Jun 12, 2024

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
Copy link
Contributor Author

pthom commented Jun 12, 2024

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 pthom force-pushed the fix_win_ci_segfault branch 4 times, most recently from c9bb147 to 9795d1d Compare June 12, 2024 10:13
@pthom
Copy link
Contributor Author

pthom commented Jun 12, 2024

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 pthom force-pushed the fix_win_ci_segfault branch from 9795d1d to 225d1bd Compare June 12, 2024 12:42
@pthom
Copy link
Contributor Author

pthom commented Jun 12, 2024

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

@Dobiasd
Copy link
Owner

Dobiasd commented Jun 12, 2024

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. 🎉

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. 😬 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
Copy link
Contributor Author

pthom commented Jun 12, 2024

CodeQL: Honestly, I have not interacted with it since it was #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
Copy link
Owner

Dobiasd commented Jun 12, 2024

Ok, that makes sense. Then, let's merge. 👍

@Dobiasd Dobiasd merged commit 624b97a into Dobiasd:master Jun 12, 2024
20 checks passed
@Dobiasd
Copy link
Owner

Dobiasd commented Jun 12, 2024

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

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