-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat(24.04): add openssh-{client,server,sftp-server} and its dependencies #142
feat(24.04): add openssh-{client,server,sftp-server} and its dependencies #142
Conversation
@rebornplusplus @cjdcordeiro Updated PR |
8ed3847
to
6e954f0
Compare
6e954f0
to
ee7c862
Compare
Diff of dependencies: slices/libc6.yaml@@ -1 +0,0 @@
-libgcc-s1 slices/libpam-modules.yaml@@ -1,10 +1,6 @@
-debconf
-debconf-2.0
libaudit1
libc6
libcrypt1
-libdb5.3t64
-libpam-modules-bin
libpam0g
libselinux1
libsystemd0 slices/libpam-runtime.yaml@@ -1,4 +0,0 @@
-cdebconf
-debconf
-debconf-2.0
-libpam-modules slices/libpam0g.yaml@@ -1,4 +1,2 @@
-debconf
-debconf-2.0
libaudit1
libc6 slices/openssh-client.yaml@@ -1,4 +1,3 @@
-adduser
libc6
libedit2
libfido2-1 slices/openssh-server.yaml@@ -1,6 +1,3 @@
-adduser
-debconf
-debconf-2.0
init-system-helpers
libaudit1
libc6 slices/passwd.yaml@@ -1,3 +1,4 @@
+base-passwd
libaudit1
libc6
libcrypt1 slices/procps.yaml@@ -1,4 +1,3 @@
-init-system-helpers
libc6
libncursesw6
libproc2-0 slices/ucf.yaml@@ -1,3 +1 @@
-debconf
-debconf-2.0
sensible-utils |
Hi Philip, will you kindly check the linting issues in the CI and the package dependencies in the above comment? I will review this shortly, before our sync on Friday. |
The linting issue is a bit annoying because I'm just writing a file verbatim from the package postinst script, I don't want to change it as the file itself is not wide (and matches what the package writes), but due to the indentation it gets a bit wider than what the linter accepts. Could we ignore it? I'll fix some of the dependencies (excluding debconf / adduser / passwd) as we can't use those to create users/debstuff anyway |
d5817e4
to
b29c4df
Compare
Hmm... we should ignore it, yeah. I will make sure to change the line-length. But since the CI is skipping another linting check, could you please run the next check locally and see if something's off? Linter script#!/bin/bash
set -e
export LC_COLLATE=C
EXIT_CODE=0
err() {
echo "error: " "$@" >&2
EXIT_CODE=1
}
warn() {
echo "warning: " "$@" >&2
}
for filename in $(find slices/ | grep "\.yaml$" | sort); do
yq '.slices | keys | .[]' "$filename" | sort -C || \
warn "$filename: slice names are not sorted"
for slice in $(yq '.slices | keys | .[]' "$filename"); do
key="$slice" yq \
'.slices | with_entries(select(.key == env(key))) | .[].essential[]' \
"$filename" | sort -C || \
err "$filename: $slice: \"essential\" entries are not sorted"
key="$slice" yq \
'.slices | with_entries(select(.key == env(key))) | .[].contents | select(.) | keys | .[]' \
"$filename" | sort -C || \
err "$filename: $slice: \"contents\" entries are not sorted"
done
done
exit $EXIT_CODE Place the script within your chisel-releases directory and run it like following:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the slices and they all are looking quite good actually. Great chiselling there, thanks! I only left a bunch of comments and suggestions that might improve the slices a bit.
Hiya @Meulengracht, about the CI, I have duplicated this PR in rebornplusplus#11. That repo has modified CI where line length issues are warnings, not errors. You do not need to do anything there, pushing your changes in this branch should update both PRs. Just make sure to check if there are any errors in that CI run. |
8477fdc
to
acf239c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the previous comments! Looks much better now.
I just have one minor nitpick about a slice name and please note that linting (excluding line-length) is failing.
57b650d
to
efdda57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thank you very much for all these slices!
This one will need to wait for another maintainer-approval. In the meantime, please feel free to open other PRs.
c91227d
to
987586b
Compare
9a9a684
to
99c1eb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(already discussed - need a rebase and conflict resolution)
99c1eb1
to
034f1a0
Compare
5d69f33
to
1597b13
Compare
e1b9504
to
089e436
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Meulengracht congrats for the successful tests :) thanks for the changes.
Could you please also add the copyright slices to each SDF? We are now making these explicit for every package, to allow for a better traceability story.
@cjdcordeiro @rebornplusplus is there more I can do? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this work @Meulengracht !
I think most comments are simply nits and design related. The actual contents look ok to me so we should be close to merge.
I feel like there's a bit of rethinking needed around the use of core
and standard
. Sometimes it feels like the standard
is delivering just the "core" functionality, other times it feels like it's delivering everything in the SDF...
7a515eb
to
b5e1006
Compare
77b6830
to
0680110
Compare
@cjdcordeiro I need a re-review |
First PR of a longer chain to add all dependencies needed to create a rootfs for core bases, as a part of switching to Chisel for building the rootfs for UC26.
This has been tested as of 22/02 and works.