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

fix: package names can be of length two #120

Merged
merged 10 commits into from
Mar 8, 2024
Merged

fix: package names can be of length two #120

merged 10 commits into from
Mar 8, 2024

Conversation

rebornplusplus
Copy link
Member

@rebornplusplus rebornplusplus commented Feb 8, 2024 •
edited
Loading

This PR adds support for package names with a minimum length of two. Previously chisel only supported a minimum length of 3. The limit on the slice name is kept unchanged.

Fixes #119.

@rebornplusplus rebornplusplus changed the title fix: package names can be of length two feat: package names can be of length two Feb 8, 2024
@rebornplusplus rebornplusplus force-pushed the fix/package-name-length branch from a195268 to 0701528 Compare February 8, 2024 09:29
@rebornplusplus
feat: package names can be of length two
0701528 
This commit adds support for package names with a minimum length of two.
Previously chisel only supported a minimum length of 3. The limit on the
slice name is kept unchanged.

Fixes canonical#119.
@cjdcordeiro cjdcordeiro added the Simple Nice for a quick look on a minute or two label Feb 8, 2024
letFunny
letFunny reviewed Feb 8, 2024
View reviewed changes
Copy link
Collaborator

@letFunny letFunny 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 link the naming requirements for debian packages if you have them? I want to see them to check the tests + regexp.

internal/setup/setup.go Show resolved Hide resolved
internal/setup/setup_test.go Outdated Show resolved Hide resolved
internal/setup/setup_test.go Outdated Show resolved Hide resolved
internal/setup/setup_test.go Show resolved Hide resolved
@letFunny
Copy link
Collaborator

letFunny commented Feb 8, 2024

If this is the correct definition (copied from the issue):

Package names (both source and binary, see Package) must consist only of lower case letters (a-z), digits (0-9), plus (+) and minus (-) signs, and periods (.). They must be at least two characters long and must start with an alphanumeric character.

Then "a-", "a+" and "a." are valid names not captured by the regexp.

@rebornplusplus
Copy link
Member Author

If this is the correct definition (copied from the issue):

Package names (both source and binary, see Package) must consist only of lower case letters (a-z), digits (0-9), plus (+) and minus (-) signs, and periods (.). They must be at least two characters long and must start with an alphanumeric character.

Then "a-", "a+" and "a." are valid names not captured by the regexp.

I only have that reference for now. I am planning to go through the Debian Policy Manual again, today. But yes, based on the definition above, you should be right. But I am not sure if a-, a+, or a. are permitted or not.

@rebornplusplus
Copy link
Member Author

rebornplusplus commented Feb 14, 2024 •
edited
Loading

So yeah, the current regex does support package names like foo+ and foo. (a+ and a. now too). But it does not support foo- (or a-).

There are packages like foo+ though:

$ grep "^Package:\s.*+$" Packages 
Package: bonnie++
Package: g++
Package: libdb5.3++
Package: memtest86+

I didn't find occurrences like foo- and foo. yet. Let me know if you spot them!

@rebornplusplus
refactor: add pkg and slice name expression string
0f724ae 
Add pkg and slice name expression string variables to remove duplication
and increase test coverage.
@rebornplusplus rebornplusplus changed the title feat: package names can be of length two fix: package names can be of length two Feb 14, 2024
@cjdcordeiro cjdcordeiro requested a review from niemeyer February 21, 2024 15:49
@lwnexgen
Copy link

lwnexgen commented Mar 6, 2024

It would be nice if this could be merged soon :) I have some weird hacks to get a static jq copied into the chiseled image.

niemeyer
niemeyer requested changes Mar 7, 2024
View reviewed changes
internal/setup/setup.go Outdated Show resolved Hide resolved
@rebornplusplus rebornplusplus requested a review from niemeyer March 8, 2024 01:00
letFunny
letFunny approved these changes Mar 8, 2024
View reviewed changes
Copy link
Collaborator

@letFunny letFunny left a comment

Choose a reason for hiding this comment

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

I still think some way of tying the three regexps together would be nice (#120 (comment)) but let's keep it simple for this bugfix.

niemeyer
niemeyer requested changes Mar 8, 2024
View reviewed changes
internal/setup/setup.go
@@ -242,9 +242,14 @@ func order(pkgs map[string]*Package, keys []SliceKey) ([]SliceKey, error) {
return order, nil
}

var fnameExp = regexp.MustCompile(`^([a-z0-9](?:-?[.a-z0-9+]){2,})\.yaml$`)
// fnameExp matches the slice definition file basename.
var fnameExp = regexp.MustCompile(`^([a-z0-9](?:-?[.a-z0-9+]){1,})\.yaml$`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for missing this earlier, but why do we have a dot (.) in that second group? That doesn't look right.

Same question below.

niemeyer
niemeyer approved these changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Let's actually go ahead with this for now. Alberto just pointed out that this is part of the package rules, and this is already in the code, so arguably an independent issue.

Let's please follow up on it, though, and agree on the path forward.

@niemeyer niemeyer merged commit 60d8b38 into canonical:main Mar 8, 2024
14 checks passed
@letFunny letFunny mentioned this pull request Apr 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@letFunny letFunny letFunny approved these changes

@lwnexgen lwnexgen lwnexgen approved these changes

@niemeyer niemeyer niemeyer approved these changes

Assignees

@niemeyer niemeyer

Labels
Simple Nice for a quick look on a minute or two
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support package names shorter than 3 characters
5 participants
@rebornplusplus @letFunny @lwnexgen @niemeyer @cjdcordeiro