Rebase Deb822 sources support (#150) onto master; fix build and a stanza-separator data loss - #274
Rebase Deb822 sources support (#150) onto master; fix build and a stanza-separator data loss#274tzh476 wants to merge 55 commits into
Conversation
…. Fix UTF-8 encoding issues.
… UTF-8 support using wide strings and codecvt_utf8 for file I/O operations.
… resolve type mismatch and build errors
…C++ wstring/string conversion helpers in Deb822 support
…rim, and use setters for RDeb822Source fields
…s.list.d/*.sources
…urces.list to Deb822 format
…ng, and Signed-By preservation
…e, preserve comments and fields, and show deb-src entries
…o prevent corruption of .sources files
… (portable, non-GTK)
…rces, add debug output for config path
…c to trace stanza and field parsing
…ser and source management
…n; preserve Deb822 flag on edit
master modernised the touched files (std:: qualification, brace reflow,
IWYU-organised includes) while this branch added Deb822 support, so most
resolutions take master's style with this branch's substance.
Two substantive hunks take this branch's version, because master's code is
superseded rather than merely restyled:
- rsources.cc write path: the sourcesByFile grouping is what the rest of the
branch's writer depends on; keeping master's filenames loop would leave two
writers in one function.
- rgrepositorywin.cc type handling: OR-ing Deb|DebSrc is the feature.
Left exactly as the author wrote them (design calls, not merge fallout):
- type_val.find("deb") also matches inside "deb-src"
- unused local orig_p in rsources.cc
- rpackagemanager.*, rsourcemanager.*, rdeb822source.* have no callers
Change-Id: I3a10cf447181a9dd6d17648afe71aa028f631384
ParseDeb822File tested line.empty(), so a separator line containing spaces or
tabs counted as content: parsing ran on into the next stanza and fields[key] =
value overwrote the previous stanza's values. The earlier source was dropped
with no error and the function still returned true.
Measured with two byte-identical inputs differing only in the separator
("\n\n" vs "\n \n"): blank yields 2 records, whitespace-only yields 1.
With this change both yield 2, with the correct URIs and suites.
CRLF files were losing sources for the same reason -- std::getline leaves
"\r" on the separator line, which was likewise treated as content. Also
verified: 2 records after, 1 before.
Unchanged by this commit: blank-separated files, the maintainer's own
debian.sources from the issue thread, empty files, comment-only files, and
Enabled: no handling.
Change-Id: I149192063d8e2644b8f4846e946be444ae964588
Regression test for the previous commit, in the same plain-main() style as the
other tests here (the branch's test_deb822_integration.cc needs gtest, which
this project does not use anywhere, and it was never wired into the build).
Verified to fail without that fix and pass with it:
without: FAIL whitespace-separator (got 1, expected 2)
FAIL tab-separator (got 1, expected 2)
FAIL crlf-separator (got 1, expected 2)
-> 3 of 9 checks failed
with: all 9 checks passed
The blank-separator case is a deliberate control: if it ever fails, the harness
is broken and the other cases prove nothing. The remaining five checks
(single stanza, trailing blank lines, comments-only, empty file, stanza missing
a required field) pin the surrounding behaviour so a later change here cannot
quietly start inventing or dropping records.
Full suite in a Debian bookworm container: 4/4 OK, 0 failures.
Change-Id: I9eb1857f8e288b84501f8444bc7eaef7dc2113ef
|
Pushed a regression test for the separator fix (
Full suite in a Debian bookworm container, from a tree extracted out of the commit itself: One note on the branch's existing CI here is showing |
Saving a .sources file erased any Architectures, Languages or Targets the user had written. rsources.cc:352 truncates the file before rewriting the stanzas, so the fields were not merely un-preserved: they were removed from the user's file on the first save. The cause was a one-sided gap rather than a missing feature. Deb822Entry already declares all three, WriteDeb822File already emits all three, and ConvertToSourceRecord/ConvertFromSourceRecord already round-trip them through record.Comment. Only ParseDeb822File never populated them, so they were always empty strings by the time the writer ran. Populating them in the parser closes the loop; no other file changes. tests/test_deb822_roundtrip.cc covers it. The test asserts on the saved file TEXT rather than on the parsed structs, because a struct comparison reads the field through the parser on both sides and so cannot see a field the parser never sets. Verified as a real control, same test both ways: without the fix: FAIL architectures / languages / targets, 3 of 4 failed with the fix: all 4 checks passed The Components case is a deliberate positive control: it is a field the parser already read, so if it ever fails the harness itself is broken. Change-Id: I252a2493bdc8c4af5cde02feb5557618cebd7fa3
Two things that both block a clean merge of this feature.
**The DEBUG prints.** Four g_print("DEBUG: ...") calls were left in
RGRepositoryEditor::Run(). They are not on master -- this branch introduced
them -- and they print to stdout for every source row on every open of the
repository dialog. Removed; pure deletions, no logic touched. Verified
against the built artifact rather than the source: `strings build/gtk/synaptic
| grep -c "DEBUG: "` goes from 4 to 0.
**The dialog path had no test.** The blocking report on this feature was
functional -- "I only get an empty window when I open the repository dialog"
-- and the list-population path was the one thing never exercised, so a green
build did not answer it.
tests/test_deb822_dialog.cc walks the same steps Run() does when it fills the
list: ReadDeb822SourcePart, the Comment skip, and the deb/deb-src type-display
logic. Fed the .sources file from that report verbatim:
row: deb, deb-src http://ftp.de.debian.org/debian/ trixie
row: deb http://security.debian.org/debian-security/ trixie-security
ok mvo5-reported-file: 2 rows
So that file does not produce an empty list on this branch. An empty input
file is included as the negative control -- it yields 0 rows, so the 2 above
cannot be an artifact of a counter that always counts.
The test stops short of constructing the GtkWindow, which needs the builder
resources and a real display; it covers the data that decides whether the
window comes up populated, which is what the report is about. Actually
clicking through the dialog still needs a human at a display.
Full suite in a Debian bookworm container: 6/6 OK, 0 build errors.
Change-Id: I550bf8a69ee0692f6dd93135841f790db03ba1d4
Same one-sided gap as the Architectures/Languages/Targets fix: everything
needed was already here except the parser.
Deb822Entry declares Comment, and WriteDeb822File already emits it in the right
place -- immediately before the stanza it belongs to (rsource_deb822.cc:125).
Only ParseDeb822File never populated it: line 66 hit '#' and continued, so the
comment was gone before the writer ran. Since rsources.cc:352 truncates the file
before rewriting the stanzas, saving deleted the user's own annotations.
deb822 has no comment syntax of its own, so apt reads '#' lines as belonging to
the stanza that follows them. This accumulates them and attaches them to the
next stanza completed, matching what the writer already assumed. A stanza
skipped for missing Types/URIs/Suites clears the accumulator, so its comment
cannot migrate onto an unrelated source.
tests/test_deb822_roundtrip.cc gains three checks. Verified both ways:
without: FAIL comment-first-stanza
FAIL comment-with-colon
FAIL comment-second-stanza
-> 3 of 7 failed
with: all 7 passed
The colon case is deliberate: comments like "# See: https://..." must not be
mistaken for a field. The second-stanza case pins the attachment rule rather
than just "some comment survived". Full suite 6/6 in a Debian bookworm
container.
Change-Id: I09f245d7821c0d666dce0cf5731e0ed7480ec7f1
…twice
ReadSources() scanned two directories: "Dir::Etc::sourcelist.d" first, then
"Dir::Etc::sourceparts". The first is not an apt configuration key. FindDir
returns "/" for it, which the old code then rewrote to a hardcoded
/etc/apt/sources.list.d/ -- the directory sourceparts already points at on a
normal system. So every .sources file was read twice and every repository was
listed twice in the repository dialog.
Measured in a sandbox with three .sources files and one legacy .list:
before: 6 records (each .sources counted twice)
after: 4 records (3 .sources + 1 .list, each exactly once)
Dropping the bogus key leaves one scan of sourceparts, which already handles
both deb822 and one-line files.
Also fixes a buffer under-read alongside it. The .sources extension test was
strcmp(Ent->d_name + strlen(Ent->d_name) - 8, ".sources")
which walks in front of the buffer for any name shorter than ".sources"; a
two-character file in sources.list.d was enough to read out of bounds. The
length is now checked first. The new test puts "ab" and "x" in the directory so
the short-name path is exercised.
tests/test_deb822_sourceparts.cc covers the enumeration itself, which is what
the "empty window" report was about -- it points apt at a temporary directory
and asserts on the records ReadSources() produces. Verified as a real control,
same test both ways:
without the fix: FAIL, got 6 records
with the fix: ok, 4 records, no duplicates
Full suite in a Debian bookworm container: 7/7 OK, 0 build errors.
Change-Id: I5547858e4c1c47746ddc84f7ee78e70694f0b399
|
@mvo5 I think I found the cause of the problem you reported on #150, and it is not in the parser. You wrote on 2025-06-02:
string Deb822Parts = _config->FindDir("Dir::Etc::sourcelist.d");
if (Deb822Parts.empty() || Deb822Parts == "/")
Deb822Parts = "/etc/apt/sources.list.d/";
...
string Parts = _config->FindDir("Dir::Etc::sourceparts");
So the first lookup always fell through to the hardcoded Dropping the bogus key leaves the single While there I also fixed a buffer under-read next to it — the extension test was
Full suite in a Debian bookworm container: 7/7 OK, 0 build errors. One caveat I want to be straight about: this explains a duplicate listing, and your report was an |
The dialog computed a display string that shows both types for a deb822
stanza, and then never used it: TYPE_COLUMN was filled from
SourceRecord::GetType(), which returns the FIRST matching type only and
cannot express "deb deb-src".
That mattered because DoEdit() parses the same column back into the record:
bool set_deb = type_val.find("deb") != npos;
bool set_debsrc = type_val.find("deb-src") != npos;
With "deb" in the column, editing any row that came from "Types: deb deb-src"
re-parsed it as deb-only, and deb-src was dropped from the user's file. The
source file in the report on this branch is exactly such a stanza.
Measured, reading the record produced by ReadSources():
before: TYPE_COLUMN "deb" -> re-parsed deb=1 deb-src=0
after: TYPE_COLUMN "deb, deb-src" -> re-parsed deb=1 deb-src=1
There were two writes to that column, not one -- the initial fill and the
repaint at the end of DoEdit(). Fixing only the first would have moved the
loss to the second edit rather than removing it, so both now go through a
single sourceTypeLabel() helper instead of a third copy of the branch.
tests/test_deb822_types_roundtrip.cc covers the round trip and fails without
the fix. Its limitation, stated plainly: the display logic lives inside a GTK
member function that needs a display to construct, so the test mirrors that
logic rather than linking it. It therefore guards the display/parse CONTRACT,
not rgrepositorywin.cc itself -- a regression in that file would not fail this
test. The parse side is the real code path's own substring test.
Full suite in a Debian bookworm container: 8/8 OK, 0 build errors.
Change-Id: I630f85727bdead38886481382e27dba0c7cf77b2
Follow-up to the deb-src fix. The label logic was a file-local helper in
gtk/rgrepositorywin.cc, which meant the test had to reimplement it: the
display path lives inside a GTK member function that needs a display to
construct, so nothing outside could call the real code. That test guarded the
contract but not the implementation -- a regression in the label logic would
not have failed it.
SourceRecord already lives in libsynaptic, which the tests link, so the logic
belongs there. It is now SourceRecord::GetTypeLabel(), the dialog calls it at
both write sites, and the test calls the same function the product calls.
Verified that this actually closes the gap, by breaking the real function and
re-running the unchanged test:
mutant (GetTypeLabel returns GetType() for the both-types case):
TYPE_COLUMN "deb" -> re-parsed deb=1 deb-src=0 FAIL
restored:
TYPE_COLUMN "deb, deb-src" -> re-parsed deb=1 deb-src=1 ok
The mirrored version of this test could not have caught that mutation, since
it carried its own copy of the branch.
No behaviour change: same strings, same two call sites.
Full suite in a Debian bookworm container: 8/8 OK, 0 build errors.
Change-Id: I2f4655599063b1a641bde26f3b0a54cce556c1b1
None of these are in common/meson.build, none are included by anything outside
their own pair, and none of the classes they declare are referenced elsewhere:
common/rpackagemanager.{cc,h} RPackageManager - 0 references outside itself
common/rsourcemanager.{cc,h} RSourceManager - 0 references outside itself
common/rdeb822source.{cc,h} RDeb822Source - 0 includes anywhere
rpackagemanager.h in particular was already removed from master in c46ca91,
"Remove the orphaned rswig.cc, rswig.h, rpackagemanager.h and rdependency.h
files"; the merge resolution reintroduced it. Restoring a file the project
deliberately deleted is not something a rebase should do silently.
rdeb822source.h is more than dead weight: it declares a SECOND class also named
RDeb822Source, distinct from the one in rsource_deb822.h that the code actually
uses. Every live include is rsource_deb822.h (rsources.cc:31, rgrepositorywin.cc:34,
rsource_deb822.cc:6, tests/test_deb822_roundtrip.cc:3). Two conflicting
declarations of one class name is a hazard even while unbuilt.
-1013 lines. Verified against a baseline in a Debian bookworm container: with the
files present 8/8 tests OK, with them removed 8/8 tests OK, 0 compile errors and
0 undefined references both ways.
Note on a flaky test: one run of test_rpackage aborted with SIGSEGV on the
reduced tree. It is not caused by this change - test_rpackage does not reference
any removed file, it links only libsynaptic, and it passed 3/3 on re-run plus a
clean full-suite pass on the same tree. It reads the container's live apt cache,
which is the likely source of the flake.
Change-Id: I22fe349549a4d2a3c8c48c0b75073f528ce5577e
tests/test_deb822_integration.cc is 324 lines that have never been compiled or
run: it is absent from tests/meson.build, and it is the only file in the whole
repository that includes <gtest/gtest.h> -- this project does not use gtest, and
it is not among the build dependencies.
It also encodes an expectation that contradicts the implementation. Its
HandleInvalidFile case asserts
EXPECT_FALSE(RDeb822Source::ParseDeb822File(testFile, entries));
for a stanza missing Suites, but ParseDeb822File deliberately SKIPS an
incomplete stanza (rsource_deb822.cc: clears fields, continues) and returns
true. So the case would fail if it were ever wired up. Shipping a test that
cannot run and would be wrong if it did is worse than shipping none.
Everything it covers is covered by tests that DO run, and the contract it got
wrong is pinned correctly by one of them:
parse / multiple entries test_deb822_separator, test_deb822_sourceparts
write-and-read-back test_deb822_roundtrip
comments test_deb822_roundtrip (3 cases incl. a colon)
extra fields test_deb822_roundtrip (Architectures/Languages/Targets)
types round-trip test_deb822_types_roundtrip
dialog population test_deb822_dialog
missing required field test_deb822_separator:89 -- asserts read returns
TRUE and yields 0 records, which is what the
implementation actually does
Full suite unchanged: 8/8 OK in a Debian bookworm container.
Change-Id: Idbbaba2f47a1971d680b22d57085c8bb2fb47b93
Two of the six deb822 tests hardcoded /tmp while the other four already did
getenv("TMPDIR") ? getenv("TMPDIR") : "/tmp"
so they fail on a build host where /tmp is read-only or redirected.
test_deb822_types_roundtrip additionally used mkdtemp's return value without
checking it for nullptr, which test_deb822_sourceparts does check. That is not
cosmetic: it turns a reportable failure into a crash.
Measured with a genuinely unwritable /tmp (a read-only tmpfs mounted over it --
chmod is not enough, since tests run as root) and TMPDIR pointing elsewhere:
before after
/tmp writable, TMPDIR unset OK / OK OK / OK
/tmp read-only, TMPDIR=/alt exit 1 / OK / OK
exit 134 SIGABRT
The exit 134 is the missing null check: mkdtemp returns nullptr, and string(root)
dereferences it. sourceparts exits 1 with a message instead, which is what both
should do.
Full suite unchanged: 8/8 OK.
Change-Id: I31a396a4bc2f22f0cd8a0335eb3f2857317ffdad
|
One more in The null check matters more than it looks. With a genuinely unwritable
Worth admitting how I nearly missed it: my first version of that experiment was Full suite: 8/8 OK. That is the third pass over my own diff, and the running total is:
Roughly half the original diff, no functionality removed, same 8 passing tests. What is left is No rush on your side — @AsciiWolf said he is short on time for a month or two, and the GUI check is |
2 participants
Rebases @aybanda's Deb822 support (#150) onto current master, and makes it build and run.
@aybanda wrote this feature — I have only rebased it and fixed what the build and tests then
found. Their PR has been unmergeable for a while (
mergeable_state: dirty) and they have not beenactive since 2025-07, so I did the rebase on a fork rather than touching their branch. If they would
rather carry it themselves, please close this and I will hand over the commits.
@mvo5, this addresses your request from 2025-05-13:
And the empty-repositories-window you reported does not reproduce. I fed your exact
debian.sourcesfrom this commentto the real
SourcesList::ReadDeb822SourcePart():1027=Deb|DebSrc(yourTypes: deb deb-src),1025=Deb. Both stanzas parse with correctURIs, suites and components.
Three commits, each droppable independently
1. Merge master (16 conflicts, 4 files). master had modernised the touched files (
std::qualification, brace reflow, IWYU includes) while the branch added Deb822, so most resolutions are
master's style with the branch's substance. Two hunks take the branch's version because master's code
is superseded, not merely restyled: the
sourcesByFilewrite path (keeping master'sfilenamesloop would leave two writers in one function), and the
Deb|DebSrcOR-ing in the type handling,which is the feature itself.
2. Build fixes. The branch predates the autotools→meson migration, so its
Makefile.amlistedrsource_deb822.ccandcommon/meson.builddid not — 8 undefined references at link. Plus<sstream>/<vector>now included explicitly inrgrepositorywin.cc(they were arrivingtransitively, so reordering includes broke
stringstream ss(...)), and one unqualifiednew string[...]→new std::string[...].3. A whitespace-only line separates deb822 stanzas — a real data-loss fix, and the one commit
here that changes behaviour, so it is easy to drop if you would rather handle it separately.
ParseDeb822Filetestedline.empty(), so a separator line containing spaces counted as content:parsing ran on into the next stanza and
fields[key] = valueoverwrote the previous one. The earliersource vanished, with no error, and the function still returned
true.Two byte-identical inputs differing only in the separator (
\n\nvs\n \n):\r)debian.sourcesEnabled: noCRLF was losing sources for the same reason —
std::getlineleaves"\r", previously treated ascontent.
Verification
Debian bookworm container, meson +
ninja -k 0, run against master as the baseline so thecomparison is checkable:
8e99020gtk/synapticlinksThe single failure is identical on both sides and environmental:
msgfmt: cannot locate ITS rules for ../data/com.ubuntu.pkexec.synaptic.policy.in. It fails onuntouched master in this container too, so it is not from these changes.
What is NOT verified
button. This needs your eyes on a real apt tree.
Left alone deliberately
Three things I could have changed and did not, because they are @aybanda's calls, not merge fallout:
URIs:/Suites:keeps only the first value, soURIs: http://a/deb http://mirror/debsilently drops the mirror. Real, but fixing it means decidinghow one stanza maps onto N
SourceRecords — a design change.type_val.find("deb")also matches inside"deb-src", soset_debis always true whenset_debsrcis. Possibly intended forTypes: deb deb-src.rpackagemanager.*,rsourcemanager.*andrdeb822source.*(~800 lines) have no callers, so theyare not in the meson build. Left in the tree rather than deleted.
Also unchanged: an unused local
orig_pinrsources.cc, which is why the warning count goes 27→28.Disclosure: this rebase, the fixes and the test harness were done with AI assistance (Claude). Every
number above comes from a run I can reproduce on request — the harness and the nine
.sourcesfixtures are available if you want them in
tests/.