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

Building firectl requires at least Go 1.14 #61

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

simonis
Copy link
Contributor

@simonis simonis commented Feb 5, 2021

It looks like building firectl requiers at least Go 1.14. Othwise I get the following build error:

$ make build-in-docker
docker run --rm -v /priv/simonisv/OpenJDK/Git/firectl:/firectl --workdir /firectl golang:1.12 make
Unable to find image 'golang:1.12' locally
1.12: Pulling from library/golang
...
# github.com/hashicorp/go-multierror
/go/pkg/mod/github.com/hashicorp/go-multierror@v1.1.0/multierror.go:112:9: undefined: errors.As
/go/pkg/mod/github.com/hashicorp/go-multierror@v1.1.0/multierror.go:117:9: undefined: errors.Is
note: module requires Go 1.14
make: *** [Makefile:30: firectl] Error 2

The fix is trivial. I've updated the Makefile and Readme.md to use the new version.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@kzys
Copy link
Contributor

kzys commented Feb 6, 2021

Thanks! That is surprising since we haven't upgraded multierror on Firecracker Go SDK because of that (firecracker-microvm/firecracker-go-sdk#223).

Do you know who is using multierror v1.1? I'm slightly inclined to downgrade multierror instead.

@simonis
Copy link
Contributor Author

simonis commented Feb 8, 2021

Thanks! That is surprising since we haven't upgraded multierror on Firecracker Go SDK because of that (firecracker-microvm/firecracker-go-sdk#223).

Do you know who is using multierror v1.1? I'm slightly inclined to downgrade multierror instead.

Not sure. I'm not a Go expert at all. I just realized the build error and found that building with 1.14 fixes it. I tried to remove the dependencies on multierror v1.1 from go.sum but when building they get re-added. I have no idea why, who requires that version and how I could find out.

The only thing I found is that the dependency was introduced by #57 (Jailer support)
What do you suggest?

@kzys kzys mentioned this pull request Feb 18, 2021
README.md Outdated Show resolved Hide resolved
@Kern--
Copy link
Contributor

Kern-- commented Oct 15, 2021

We recently decided to take a dependency that requires Go 1.14 in firecracker-go-sdk, so I think this PR is OK to move forward (firecracker-microvm/firecracker-go-sdk#363).

@simonis If you're still available to work on this PR can you please do the following?

  1. Address the small comment
    and
  2. Add a signed off line to your commit (e.g. by commiting using git commit -s)

@simonis
Copy link
Contributor Author

simonis commented Oct 21, 2021

Hi @Kern-- ,

yes, I'm still around, I just didn't saw your comment :)

I've pulled and merged with upstream, changed the comment in Readme.md as requested and signed off the commit.

Please let me know if there's anything else I can do?

Best regards,
Volker

@austinvazquez
Copy link
Contributor

@simonis, this looks good to me. To pass the DCO validation, you'll need to squash the commits locally and force push to your branch.

Signed-off-by: Volker Simonis <volker.simonis@gmail.com>
@simonis
Copy link
Contributor Author

simonis commented Jul 12, 2022

Hi @austinvazquez,
I've done as requested.
Please let me know if there's anything else I have to do.
Best regards,
Volker

Copy link
Contributor

@austinvazquez austinvazquez left a comment

Choose a reason for hiding this comment

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

LGTM!

@austinvazquez austinvazquez merged commit 94ac8f8 into firecracker-microvm:main Jul 12, 2022
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.

4 participants