Skip to content

refactor log package to use hashicorp/go-syslog to support windows #6021

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

Closed
wants to merge 2 commits into from

Conversation

cpanato
Copy link
Contributor

@cpanato cpanato commented Mar 30, 2022

In the change sigstore/cosign#1676 introduce the dependency for boulder and we compile the tool for several target archs and one is windows, looks like the "log/syslog" does not have support for windows, however, there is a project from hashicorp https://github.com/hashicorp/go-syslog that helps on this side to be same to cross-compile.

I did a update in our side in this testing PR and tested our tool and seems to work fine: cpanato/cosign#175

Hopefully, this is ok :) Let me know what your thoughts on this

This PR updates the log package to use github.com/hashicorp/go-syslog and adjust where that is used.

@jsha
Copy link
Contributor

jsha commented Mar 30, 2022

Hi! Glad you've found our goodkey package useful.

I'm a little confused about why our logging infrastructure is introducing problems for you. The goodkey package doesn't depend on syslog, and as I understand it, Go only compiles the transitive graph of packages you depend on, not whole modules, right? What's the error you're getting related to syslog, and what's the command you're running?

@cpanato
Copy link
Contributor Author

cpanato commented Mar 30, 2022

that is a good question, I did not look deep into the changes but maybe @haydentherapper can help me here

@haydentherapper
Copy link

I'm not super familiar with how Go compiles packages, so I might not be very helpful, but I'll summarize the changes.

We added a dependency on letsencrypt/boulder in sigstore/sigstore, a repo with common code for all Sigstore repos.

I updated sigstore/cosign to use the latest sigstore/sigstore commit, and this pulled in github.com/letsencrypt/boulder v0.0.0-20220322173223-dd8be8d7b02c as a direct dependency. Maybe this should be an indirect dependency? I'm not familiar with the build process.

@jsha
Copy link
Contributor

jsha commented Mar 30, 2022

One of the tricky distinctions in Go is between "modules" and "packages". A "module" is defined by having a go.mod, and usually corresponds to a whole repository. A "package" is always a single directory. Typically one module has many packages, as is the case in Boulder.

So when you add a dependency on the module github.com/letsencrypt/boulder, you're depending on the whole repository, which downloads and is versioned as a single unit.

However, when you build one of your packages, Go only builds the subset of packages that it transitively depends on.

And, in the course of writing that down, I realized the issue: goodkey doesn't depend directly on our log package; but it does depend on it transitively, by way of the core package.

Looking at our core package, it only calls the log package in one place: to debug-log a problem marshaling a public key. That usage shouldn't exist, since we return the error in that case and the caller can log it.

Want to try removing that log call in core and see if solves your problem? There might also be some other transitive dependencies from the goodkey package to log, but I suspect they can all be factored away. I'd prefer that over taking a new dependency.

@cpanato
Copy link
Contributor Author

cpanato commented Mar 31, 2022

closing in favor of #6029

@cpanato cpanato closed this Mar 31, 2022
@cpanato cpanato deleted the go-syslog branch March 31, 2022 09:14
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.

3 participants