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

add lint.sh and check in CI #73

Closed
wants to merge 3 commits into from
Closed

Conversation

imjasonh
Copy link
Member

@imjasonh imjasonh commented Nov 3, 2022

This adds a simple lint.sh script that processes files with yq to:

  • remove any .environment.contents.repositories and .environment.contents.keyring from Melange configs -- these packages should be explicitly pulled into the local repository and --repository-appended when Melange is invoked
  • consistently format YAML files, especially whitespace

Unfortunately this strips empty lines, which might be less than optimal for readability (and the size of this diff). If we want to handle this better, we can.

This adds a GitHub CI check that runs ./lint.sh and fails if it resulted in any diffs.

We can extend this to perform more checks if we want them. I'd like to make sure the Makefile contains a line for every package-version-epoch.apk, for example. More ideas welcome.

Signed-off-by: Jason Hall [email protected]

@imjasonh imjasonh requested review from kaniini and luhring November 3, 2022 14:36
curl.yaml Outdated
--with-pic \
--disable-ldap \
--without-libssh2
opts: "--enable-ipv6 \\\n--enable-unix-sockets \\\n--with-openssl \\\n--with-nghttp2 \\\n--with-pic \\\n--disable-ldap \\\n--without-libssh2 \n"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is sort of unfortunate 😒

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, let's not.

The easiest way will be to just have it be a single line, which is unambiguously formatted. Let me know if you'd like me to figure out how to get the newlines preserved instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to keep the newlines as they are.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would also prefer that. Unfortunately I can't figure out how to get yq to preserve those. Short of writing our own yamlfmt or adding support for this to yq, I think it's a worthy sacrifice if it means we get linting and bug catching in place.

ncurses.yaml Show resolved Hide resolved
@kaniini
Copy link
Contributor

kaniini commented Nov 3, 2022

remove any .environment.contents.repositories and .environment.contents.keyring from Melange configs -- these packages should be explicitly pulled into the local repository and --repository-appended when Melange is invoked

This should only be removing the https://packages.wolfi.dev/os repository, right? We do need to specify the stage3 repository for the early packages where we are still lifting ourselves out of bootstrap world.

Signed-off-by: Jason Hall <[email protected]>
@imjasonh
Copy link
Member Author

imjasonh commented Nov 3, 2022

remove any .environment.contents.repositories and .environment.contents.keyring from Melange configs -- these packages should be explicitly pulled into the local repository and --repository-appended when Melange is invoked

This should only be removing the https://packages.wolfi.dev/os repository, right? We do need to specify the stage3 repository for the early packages where we are still lifting ourselves out of bootstrap world.

Yeah that makes sense. I had thought we were past the bootstrapping phase enough that we didn't need these anymore, but if there are things we still need from there then we should keep them. I've updated the script to only remove packages.wolfi.dev/os.

Is there a list of things we need to do to fully exit bootstrapping? It will be easier to tell our build is hermetic when we can show we don't have any external repos, including bootstrapping.

@kaniini
Copy link
Contributor

kaniini commented Nov 4, 2022

Is there a list of things we need to do to fully exit bootstrapping? It will be easier to tell our build is hermetic when we can show we don't have any external repos, including bootstrapping.

In practice, for x86_64, we have exited bootstrapping, but we want to keep the distribution bootstrappable. This is helpful for adding additional archs, and also allowing others to reproduce the entire distribution from scratch.

We can probably have a setting via the melange templating system to ignore the repos when not in bootstrap mode?

@imjasonh
Copy link
Member Author

imjasonh commented Nov 4, 2022

In practice, for x86_64, we have exited bootstrapping, but we want to keep the distribution bootstrappable. This is helpful for adding additional archs, and also allowing others to reproduce the entire distribution from scratch.

We can probably have a setting via the melange templating system to ignore the repos when not in bootstrap mode?

Could we make the configs bootstrappable again by adding MELANGE_EXTRA_OPTS=--repository-append=https://packages.wolfi.dev/bootstrap/stage3?

If these are left as markers for where bootstrapping comes into play then that's great and we can leave it.

@kaniini
Copy link
Contributor

kaniini commented Nov 4, 2022

My intent is to leave it as a marker, but also I don’t want to append the repository to every build, just the bootstrapped ones.

@imjasonh
Copy link
Member Author

imjasonh commented Nov 4, 2022

lol #80 means glibc.yaml isn't YAML anymore, so yq can't parse or format it.

Going back to the drawing board folks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants