Skip to content

[webgui] testing /snap/bin/chromium #18311

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

Merged
merged 3 commits into from
Apr 10, 2025
Merged

Conversation

linev
Copy link
Member

@linev linev commented Apr 8, 2025

Partially resolves https://root-forum.cern.ch/t/63182/
Let use snap installation of chromium on the platform where pre-compiled ROOT version is used

Also add special handling of temporary directory.
snap version of chromium has limitation that files from /tmp
cannot be accessed. Therefore if TMPDIR is not set, try to use directly home directory.

Adjust batch code to use new functionality with temp directory

Delete temporary HTML file as soon as connection is established

@linev linev requested a review from bellenot April 8, 2025 11:50
@linev linev self-assigned this Apr 8, 2025
Copy link

github-actions bot commented Apr 8, 2025

Test Results

    17 files      17 suites   3d 22h 47m 58s ⏱️
 2 688 tests  2 688 ✅ 0 💤 0 ❌
45 060 runs  45 060 ✅ 0 💤 0 ❌

Results for commit 716edfc.

♻️ This comment has been updated with latest results.

linev added 3 commits April 10, 2025 08:27
Try to detect snap version before normal one.
Partially resolves https://root-forum.cern.ch/t/63182/
Let use snap installation of chromium on the platform where precompiled ROOT version is used
/snap/bin/chromium has restriction to access general temporary directory. Therefore if TMPDIR is not set, try to use directly home directory.

Also in some other installations chromium
may have restriction to access HTML files from tmp dir. Code was exists already for batch case - now extend it as well.
Temporary file required to start browser.
Then connection established - information is not longer necessary.
Also increase security.
@couet couet self-requested a review April 10, 2025 07:54
@linev linev merged commit 2361862 into root-project:master Apr 10, 2025
20 of 23 checks passed
@hahnjo
Copy link
Member

hahnjo commented Apr 10, 2025

@linev this broke Windows builds, as shown by the CI in this PR already:

RWebDisplayHandle.cxx(145,45): error C2146: syntax error: missing ')' before identifier 's'

I'm fixing this in #18356...

@linev
Copy link
Member Author

linev commented Apr 11, 2025

@hahnjo

Yes, I applied clang-format and didnot check result.
Thanks for fixing it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants