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

apt: Only backup/restore sources.list if it exists #1885

Merged
merged 3 commits into from Jan 24, 2024

Conversation

julian-klode
Copy link
Contributor

We backup the sources.list to a file in sources.list.d to be able to insert our source first; if we do not have a sources.list this is not necessary.

If an image uses ubuntu.sources, this logic also works fine as sources.list that we write will be sourced before the ubuntu.sources.

We backup the sources.list to a file in sources.list.d to be
able to insert our source first; if we do not have a sources.list
this is not necessary.

If an image uses ubuntu.sources, this logic also works fine as
sources.list that we write will be sourced before the ubuntu.sources.
@julian-klode
Copy link
Contributor Author

I haven't tested this, this is a proof of concept. I actually need to learn again how to build images yet to test the changes in https://code.launchpad.net/~juliank/livecd-rootfs/+git/livecd-rootfs/+merge/458366

@ogayot ogayot self-requested a review January 15, 2024 16:14
@julian-klode
Copy link
Contributor Author

Reading this more I'm not sure if this actually deletes the /etc/apt/sources.list with the cdrom line, I guess the _restore_file is weird and should always succeed or something.

@ogayot
Copy link
Member

ogayot commented Jan 15, 2024

Related to LP:#2026647

Copy link
Member

@ogayot ogayot left a comment

Choose a reason for hiding this comment

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

Thanks Julian!

Reading this more I'm not sure if this actually deletes the /etc/apt/sources.list with the cdrom line, I guess the _restore_file is weird and should always succeed or something.

Yes, the presence of etc/apt/sources.lists containing the cdrom entry is unconditional

write_file(
self.install_tree.p("etc/apt/sources.list"),
f"deb [check-date=no] file:///cdrom {codename} main restricted\n",
mode=0o644,

I'm not sure if it would be a huge deal to leave this file on the target system, but for now, I think it's better to keep the current behavior - which means removing the file if it does not exist in the source overlay.

subiquity/server/apt.py Outdated Show resolved Hide resolved
@ogayot ogayot requested a review from dbungert January 22, 2024 11:55
Copy link
Member

@ogayot ogayot left a comment

Choose a reason for hiding this comment

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

I pushed a change to drop sources.list from the target if it wasn't present in the configured tree.

Also added a set of tests for the .deconfigure method.

Marking "Approved" since I requested changes earlier. But it would be fair to get a second approval before merging since I authored some of the changes.

Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Back when sources.list was always present, we would:
 * move the contents of sources.list to sources.list.d/original.list
 * write the CDROM info to sources.list so the CDROM cat be used as an
   APT source
 * restore the original sources.list file when .deconfigure() is called,
   effectively discarding CDROM info

Nowadays, there is no guarantee that sources.list exists. So restoring
the contents of sources.list may mean deleting sources.list if it wasn't
present in the first place.

Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Copy link
Contributor

@Chris-Peterson444 Chris-Peterson444 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@mwhudson mwhudson left a comment

Choose a reason for hiding this comment

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

Thanks for this, and for adding tests!

@mwhudson mwhudson merged commit 162f00b into canonical:main Jan 24, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants