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

Fix Anaconda startup on Live media via custom webview launcher script #4854

Merged

Conversation

M4rtinK
Copy link
Contributor

@M4rtinK M4rtinK commented Jun 22, 2023

This should fix the startup on Live images for now.

The first commit reverts the revert & switches Anaconda to again use the custom version of cockpit-desktop, called webui-desktop.

The second commit updates the script to the version found in the latest cockpit-ws package.

set -eu

# exec_prefix= is set because the default /usr/libexec contains "${exec_prefix}"
exec_prefix="/usr"

Check warning

Code scanning / shellcheck

exec_prefix appears unused. Verify use (or export if used externally).

exec_prefix appears unused. Verify use (or export if used externally).

# exec_prefix= is set because the default /usr/libexec contains "${exec_prefix}"
exec_prefix="/usr"
libexecdir="/usr/libexec"

Check warning

Code scanning / shellcheck

libexecdir appears unused. Verify use (or export if used externally).

libexecdir appears unused. Verify use (or export if used externally).

# start the bridge; this needs to run in the normal user session/namespace
coproc ${2:+ssh "$2"} cockpit-bridge
trap "kill $COPROC_PID; wait $COPROC_PID || true" EXIT INT QUIT PIPE

Check warning

Code scanning / shellcheck

Use single quotes, otherwise this expands now rather than when signalled.

Use single quotes, otherwise this expands now rather than when signalled.

# start the bridge; this needs to run in the normal user session/namespace
coproc ${2:+ssh "$2"} cockpit-bridge
trap "kill $COPROC_PID; wait $COPROC_PID || true" EXIT INT QUIT PIPE

Check warning

Code scanning / shellcheck

Use single quotes, otherwise this expands now rather than when signalled.

Use single quotes, otherwise this expands now rather than when signalled.
@M4rtinK M4rtinK changed the title Master webui hotfix cockpit desktop Fix Anaconda startup on Live media via custom webview launcher script Jun 22, 2023
Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me otherwise.

@@ -57,6 +58,7 @@ install-data-hook: $(WEBPACK_TEST)
cp -r dist/* $(DESTDIR)/usr/share/cockpit/$(PACKAGE_NAME)
mkdir -p $(DESTDIR)/usr/share/metainfo/
cp org.cockpit-project.$(PACKAGE_NAME).metainfo.xml $(DESTDIR)/usr/share/metainfo/
cp webui-desktop $(DESTDIR)/usr/libexec/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this go to /usr/libexec/anaconda/ instead?

Copy link
Contributor Author

@M4rtinK M4rtinK Jun 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a re-revert of working code - I suggest we handle non-critical improvements in separate PR.

@@ -5,4 +5,5 @@ ignore_patterns=(
dockerfile/
scripts/
widgets/
webui-desktop # This is just a temporary file, to be replaced with cockpit-desktop once 'unshare' is not filtered out by lorax
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasoning is wrong, unshare is now present
weldr/lorax#1222

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the reasoning.

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also pls write in the commit message what was the problem with the unshare command.

@M4rtinK
Copy link
Contributor Author

M4rtinK commented Jun 23, 2023

Can you also pls write in the commit message what was the problem with the unshare command.

So I have this in the first commit message:

Unfortunately it turns out the unshare command prevents the Web UI from being started on Live images, so we need to put this temporary hotfix back in.

As for why it does not work - we have not investigated further so far. It was actually the Image Builder team finding out it start to work if we drop the unshare command from the cockpit-desktop script.

@M4rtinK M4rtinK force-pushed the master-webui_hotfix_cockpit_desktop branch from d2612e3 to 2b17655 Compare June 23, 2023 14:40
@M4rtinK
Copy link
Contributor Author

M4rtinK commented Jun 23, 2023

Updated the PR.

@M4rtinK
Copy link
Contributor Author

M4rtinK commented Jun 23, 2023

@martinpitt @allisonkarlitskaya @ondrejbudai @supakeen So basically during early Image Builder built Live image we found out the Anaconda Web UI won't start as long as unshare is used by the cockpit-desktop script. Once we manually removed it, Web UI started as expected. So for now we decided to go back to the forked cockpit-desktop script temporarily, but it would be certainly good to think about a more permanent solution eventually. Suggestions Welcome. :)

…d of cockpit-desktop""

This reverts commit 94325bc.

Unfortunately it turns out the unshare command prevents the Web UI from
being started on Live images, so we need to put this temporary hotfix
back in.
Taken from cockpit-294.1-2.fc39, minus the unshare invocation.
@M4rtinK M4rtinK force-pushed the master-webui_hotfix_cockpit_desktop branch from 2b17655 to 996b0a2 Compare June 26, 2023 13:15
@M4rtinK
Copy link
Contributor Author

M4rtinK commented Jun 26, 2023

/kickstart-test --testtype smoke

@jkonecny12
Copy link
Member

Feel free to ignore these issues:

  • cppcheck is failing on code from gettext-devel package: reported here
  • ShellCheck is failing on webui-desktop which is Cockpit code and should be working
  • Testing Farm is failing on DNF5 incompatibility issues

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me as hotfix.

@M4rtinK M4rtinK merged commit 05577a8 into rhinstaller:master Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants