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.

Sign up for GitHub

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

Jump to bottom

Copy /var/lib/gnome-initial-setup/state to installed system #5056

Merged
merged 1 commit into from
Sep 7, 2023
Merged

Copy /var/lib/gnome-initial-setup/state to installed system #5056

merged 1 commit into from
Sep 7, 2023

Conversation

halfline
Copy link
Contributor

This is one more bit needed for:

https://pagure.io/fedora-workstation/issue/362

The idea is to copy over which pages the user already visited on the live system so we don't reshow them on the installed system.

I'm not entirely clear on the difference between the live_os payload module and the live_image payload module. I suspect I only need to change the former, but clarity would be appreciated. I did both.

I haven't really tested it yet. I'm building an image now to try it out.

@github-actions github-actions bot added the f39 label Aug 17, 2023
@halfline
Copy link
Contributor Author

I'm not entirely clear on the difference between the live_os payload module and the live_image payload module. I suspect I only need to change the former, but clarity would be appreciated. I did both.

Okay, given generating a live image just failed with this patch but installing a live image succeed, i'm going to guess the difference between live_os and live_image payload is the latter is for making the iso. I'll drop that part from the patch

@halfline halfline force-pushed the copy-gnome-state-over branch 2 times, most recently from 46334bf to a20c80d Compare August 19, 2023 03:39
@halfline
Copy link
Contributor Author

i added a commit to copy some of the dconf state over as well.

@halfline halfline force-pushed the copy-gnome-state-over branch 3 times, most recently from 48ac3a3 to d5bde34 Compare August 20, 2023 00:34
@VladimirSlavik
Copy link
Contributor

VladimirSlavik commented Aug 22, 2023 •
edited
Loading

  • You have path(s) as parameters to the task constructor, but these are in both cases known and static. Just hardcode the list inside the task.
  • The general copying task is a no-go, please name it as it is.
  • The tasks need to run only on live. The pattern is to check conf.system.provides_liveuser and return early in run(), I think. I need to re-check though, maybe that's ok since these run for the live payloads only.
  • I'll have to look at where to put these tasks, live_os vs. live_image.
  • There are no tests?

VladimirSlavik
VladimirSlavik reviewed Aug 22, 2023
View reviewed changes
pyanaconda/modules/payloads/payload/live_image/installation.py Outdated
Comment on lines 534 to 553
process = startProgram(["dconf", "dump", path], stderr=subprocess.PIPE, env_prune=["USER", "LOGNAME", "HOME"], user=self._uid)
stdout, stderr = process.communicate()

if process.returncode != 0:
log.debug(f"dconf dump {path} failed: {stderr.decode('utf-8')}")
continue

lines = stdout.decode('utf-8').split('\n')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should use execWithCapture?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks like there's a execWithCaptureAsLiveUser but using it would require landing #5074 first i think.

@VladimirSlavik
Copy link
Contributor

I think I should be able to take over this and anacondify it, if the code is mostly final... but not promising yet.

@halfline halfline closed this Aug 22, 2023
@halfline halfline reopened this Aug 22, 2023
@halfline
Copy link
Contributor Author

I think I should be able to take over this and anacondify it, if the code is mostly final... but not promising yet.

Sure if you want to to take it over fine with me.

@jkonecny12
Copy link
Member

Hi @halfline the input sources shouldn't be required. It should be handled by #4976 . If this doesn't work than we should find a reason why.

Another thing is that these changes will fail on spins so it has to be conditionalized.

jkonecny12
jkonecny12 requested changes Aug 22, 2023
View reviewed changes
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.

We shouldn't merge the second commit. Changes implemented to Anaconda before should work.

@AdamWill
Copy link
Contributor

Filed an RHBZ for blocker/FE tracking purposes.

@halfline
Copy link
Contributor Author

We shouldn't merge the second commit. Changes implemented to Anaconda before should work.

So it's actually probably still a good idea to have it. We're okay without it for cases where there is only one input method for a given language, but take say Chinese for instance... The user could pick "Pinyin" or "Bopomofo". It would be good if they chose Pinyin if they didn't end up with Bopomofo on reboot.

This is a problem unique to the live images, because boot.iso doesn't allow input method selection at all right?

@AdamWill
Copy link
Contributor

Yes, that's correct.

@halfline
Copy link
Contributor Author

i've made a few changes to deal with feedback. I haven't tested the changes yet, they're just me editing things in vim. but will try to spin up an iso soon to test them out.

@VladimirSlavik
Copy link
Contributor

/kickstart-test --testtype smoke

@VladimirSlavik
Copy link
Contributor

Looks like some import is wrong?

@halfline
Copy link
Contributor Author

iso failed, I forgot to do one rename. should be fixed now.

@VladimirSlavik
Copy link
Contributor

/kickstart-test --testtype smoke

@VladimirSlavik
Copy link
Contributor

Looks good so far, the only test fail is for amount of tasks which is now different, I'll take care of that with the rest of tests.

@VladimirSlavik
Copy link
Contributor

VladimirSlavik commented Sep 4, 2023 •
edited
Loading

Push above was rebase to master + tests + some other fixes. This should pass unit tests etc. Still todo - look up where should this actually run:

I'll have to look at where to put these tasks, live_os vs. live_image.

@VladimirSlavik
Copy link
Contributor

/kickstart-test --testtype smoke

@halfline @VladimirSlavik
Copy /var/lib/gnome-initial-setup/state to installed system
0d2279a 
In order to avoid duplicate screens in gnome-initial-setup we need
to let the gnome-initial-setup on the installed system know what
the user did during the live boot.

This commit copies the relevant state file over.
@VladimirSlavik
Copy link
Contributor

/kickstart-test --testtype smoke

@VladimirSlavik
Copy link
Contributor

I removed the second commit based on #5056 (review). The previous state of the PR is attached: 5056.original.zip

VladimirSlavik
VladimirSlavik approved these changes Sep 5, 2023
View reviewed changes
Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

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

I think this should be ok.

@VladimirSlavik
Copy link
Contributor

@halfline does this need further testing, or did you test the first part already? I think we're good to go save for knowing it works :)

@halfline
Copy link
Contributor Author

halfline commented Sep 5, 2023

we really need the second commit for things to work right with Chinese. see #5056 (comment)

jkonecny12
jkonecny12 approved these changes Sep 7, 2023
View reviewed changes
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 great to me without the second part. Thanks @halfline and @VladimirSlavik for focusing just on this in the PR. I'll check the second part when I'm able to.

@jkonecny12
Copy link
Member

we really need the second commit for things to work right with Chinese. see #5056 (comment)

Why do you think so. If we merge this with the current form users with these specific layouts just need to set the language manually after the installation. I still see these as two bugs and one is not breaking the other. Yes, we should ideally add that but that could be handled separately IMHO.

@VladimirSlavik VladimirSlavik merged commit 1ef9461 into rhinstaller:master Sep 7, 2023
16 checks passed
@VladimirSlavik VladimirSlavik mentioned this pull request Sep 7, 2023
Merged
@halfline
Copy link
Contributor Author

halfline commented Sep 7, 2023

we really need the second commit for things to work right with Chinese. see #5056 (comment)

Why do you think so. If we merge this with the current form users with these specific layouts just need to set the language manually after the installation. I still see these as two bugs and one is not breaking the other. Yes, we should ideally add that but that could be handled separately IMHO.

Agreed we can handle this post-beta, but my assumption is the situation is like a user is choosing the Chinese input method analog of "dvorak" prior to install and then booting up post install to find the system is configured for the Chinese input method analog of "qwerty" (or vice versa).

anyway we'll get sorted post beta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkonecny12 jkonecny12 jkonecny12 approved these changes

@VladimirSlavik VladimirSlavik VladimirSlavik approved these changes

Assignees
No one assigned
Labels
f40
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@halfline @VladimirSlavik @jkonecny12 @AdamWill