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

fix: remove implicit copyright installation #170

Merged
merged 1 commit into from
Nov 22, 2024
Merged

fix: remove implicit copyright installation #170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions internal/slicer/slicer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,6 @@ func Run(options *RunOptions) error {
extract[slice.Package] = extractPackage
}
arch := pkgArchive[slice.Package].Options().Arch
copyrightPath := "/usr/share/doc/" + slice.Package + "/copyright"
hasCopyright := false
for targetPath, pathInfo := range slice.Contents {
if targetPath == "" {
continue
Expand All @@ -119,9 +117,6 @@ func Run(options *RunOptions) error {
Path: targetPath,
Context: slice,
})
if sourcePath == copyrightPath && targetPath == copyrightPath {
hasCopyright = true
}
} else {
// When the content is not extracted from the package (i.e. path is
// not glob or copy), we add a ExtractInfo for the parent directory
Expand All @@ -136,12 +131,6 @@ func Run(options *RunOptions) error {
})
}
}
if !hasCopyright {
extractPackage[copyrightPath] = append(extractPackage[copyrightPath], deb.ExtractInfo{
Path: copyrightPath,
Optional: true,
})
}
}

// Fetch all packages, using the selection order.
Expand Down
8 changes: 1 addition & 7 deletions internal/slicer/slicer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ var slicerTests = []slicerTest{{
"/dir/text-file-3": "file 0644 5b41362b {test-package_myslice}",
},
}, {
summary: "Copyright is installed",
summary: "Copyright is not installed implicitly",
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need tests for every behavior we don't have. :)

Can we please just drop the test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was a "regression test", in a way it is to show that the PR is working correctly. Should I remove it and depend on manual testing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand. Is there any special behavior left, or is copyright handling now exactly the same as everything else? What is left to test that is not business as usual?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is handled normally indeed, what we are testing is that in fact we are not installing the copyright implicitly anymore because we have removed all of the logic and not only partially. Basically, that the PR works as intended and the relevant logic is removed. If I remove the test then we would have to rely on me testing locally that the copyright is not implicitly installed anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

Per our conversation, we're only doing this because inverting the test was trivial, but in general we don't test an "anti-feature". That is, if you remove a complex feature, you typically drop the tests and the implementation, rather than doing that and also adding a selection of new tests that adds more logic to ensure that the behavior was inverted, when that behavior is actually the typical behavior already tested everywhere else.

slices: []setup.SliceKey{{"test-package", "myslice"}},
pkgs: []*testutil.TestPackage{{
Name: "test-package",
Expand All @@ -265,12 +265,6 @@ var slicerTests = []slicerTest{{
filesystem: map[string]string{
"/dir/": "dir 0755",
"/dir/file": "file 0644 cc55e2ec",
// Hardcoded copyright entries.
"/usr/": "dir 0755",
"/usr/share/": "dir 0755",
"/usr/share/doc/": "dir 0755",
"/usr/share/doc/test-package/": "dir 0755",
"/usr/share/doc/test-package/copyright": "file 0644 c2fca2aa",
},
manifestPaths: map[string]string{
"/dir/file": "file 0644 cc55e2ec {test-package_myslice}",
Expand Down
Loading