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

Transitive dependency (cloudflare/circl) making cross compiling difficult #2932

Closed
pfiaux opened this issue Sep 18, 2023 · 7 comments · Fixed by #2935
Closed

Transitive dependency (cloudflare/circl) making cross compiling difficult #2932

pfiaux opened this issue Sep 18, 2023 · 7 comments · Fixed by #2935

Comments

@pfiaux
Copy link

pfiaux commented Sep 18, 2023

I'm running into a slightly obscure build problem. Using bazel (and gazelle and rules_go) to build go-github worked fine with v50 both for arm64 and amd64. I recently tried to bump to v55 and found myself unable to build for amd64 via bazel.

I found that the transitive dependency github.com/cloudflare/circl was bumped from v1.1.0 to v1.3.3 after v52 of go-github. It seems used by github.com/ProtonMail/go-crypto.

The problem is that it includes some platform specific files (*_amd64.go) for some packages, and some headers and some assembly.

Unfortunately led to some issues trying to build via bazel with dependencies not detected:

external/com_github_cloudflare_circl/dh/x25519/curve_amd64.s:7: #include: open external/com_github_cloudflare_circl/math/fp25519/fp_amd64.h: no such file or directory

Quickly playing with GOARCH=amd64 it seems to work fine with go build so I suspect it's not actually a problem but more of a side effect of bazel trying to compile a bit too much of some of the libraries used transitively.

Since CGO usually raises the bar for compiling I thought I would mention that it might not be ideal that github.com/ProtonMail/go-crypto uses github.com/cloudflare/circl:

  • Depending on the architecture it might need CGO for some packages
  • https://github.com/cloudflare/circl claims to aim to be an experimental lib which might maybe not ideal to depend on transitively

    The goal of this library is to be used as a tool for experimental deployment of cryptographic algorithms targeting Post-Quantum (PQ) and Elliptic Curve Cryptography (ECC)

Is there an alternative to github.com/ProtonMail/go-crypto that would avoid this?

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 18, 2023

Triage info:

Still investigating...

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 18, 2023

I'm wondering if you might get more traction by posting an issue here: https://github.com/ProtonMail/go-crypto/issues ?
Maybe one of their users has already encountered this and has a workaround?

@WillAbides
Copy link
Contributor

If there is an appetite to change the API, we could replace Commit.SigningKey with a single-method interface or a Sign func that looks like:

Sign func(w io.Writer, r io.Reader) error

Then go-github wouldn't need to depend on any openpgp implementation.

Devs who want to sign commits would need to do something like:

c.Sign = func(w io.Writer, r io.Reader) error {
	return openpgp.ArmoredDetachSign(w, myKey, r, nil)
}

That's a little bit worse than the current experience, but not terrible.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 18, 2023

I'm totally fine with that, as ProtonMail seems to me like a rather unusual dependency for this repo anyway.

@cablehead - @hungle92 - @exageraldo - @komod0 - do you have opinions since you were all involved in the issues linked above?

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 19, 2023

Thank you, @exageraldo - moving forward with #2935 hearing no objections.

@ibexmonj
Copy link

Can someone elaborate what the the fix here is for the OP's issues ? I am running into the same issue and unsure how to proceed

@gmlewis
Copy link
Collaborator

gmlewis commented Oct 29, 2024

Can someone elaborate what the the fix here is for the OP's issues ? I am running into the same issue and unsure how to proceed

@ibexmonj - does #2935 give you enough information on how to update your client?

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 a pull request may close this issue.

4 participants