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

chore: update ragel version used by CI #1245

Closed
wants to merge 1 commit into from

Conversation

wolffcm
Copy link

@wolffcm wolffcm commented May 6, 2019

This was causing headaches because most users are using ragel 6.10 on
local machines, but the Docker image used 6.9 (several years old now).

Fixes #1183.

Done checklist

  • docs/SPEC.md updated
  • Test cases written

@wolffcm wolffcm requested a review from nathanielc May 6, 2019 19:22
@wolffcm
Copy link
Author

wolffcm commented May 6, 2019

@nathanielc I was able to build and test this new Dockerfile_build locally and it seems okay, but CI is not picking it up. Do I need to update the image nathanielc/flux-build:latest in a Docker registry somewhere?

@nathanielc
Copy link
Contributor

nathanielc commented May 6, 2019

@wolffcm Yes, its a bit backwards, but the source of the image is the Dockerfile_build from the Flux repo, but the docker image CI uses will not be updated till the changes are merged to master. So its a bit of chicken and egg issue.

One way to solve would be to split this PR, one PR to change the docker image and one for the changes to the generated scanner. The docker image PR will go green, since it doesn't really change anything in CI. Then the second PR can be merged. The only catch is that no other PRs will go green until the changes to the scanner are merged.

@wolffcm wolffcm force-pushed the chore/udpate-ragel-ci-version branch from 09984f6 to f44a029 Compare May 6, 2019 21:28
wolffcm pushed a commit that referenced this pull request May 6, 2019
@wolffcm wolffcm mentioned this pull request May 6, 2019
2 tasks
@wolffcm
Copy link
Author

wolffcm commented May 6, 2019

@nathanielc Ok, makes sense. I made this PR just update the dockerfile, and created a separate PR for updating the generated code:
#1246

git \
make \
gcc \
libc-dev \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are gcc and libc-dev needed?

Copy link
Author

Choose a reason for hiding this comment

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

I found that go vet seems to use CGO, so it needs them. We wouldn't need them if we set CGO_ENABLED=0 in the environment. Do you have a preference?

Copy link
Author

Choose a reason for hiding this comment

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

I did a little more digging on this issue, I think that go vet needs to build something for some reason, and through transitive dependencies something that needs CGO (I seem to recall that the net package uses CGO) gets compiled.

This is a recent change in Go 1.11 that seems not to have been fixed in Go 1.12:
golang/go#26988

Maybe the best way forward would be to add CGO_ENABLED=0 to the environment, and if/when that Go bug gets fixed, we won't have any unneeded dependencies in the docker build.

@wolffcm wolffcm force-pushed the chore/udpate-ragel-ci-version branch from f44a029 to 76bf82e Compare May 7, 2019 17:27
@wolffcm wolffcm force-pushed the chore/udpate-ragel-ci-version branch from 76bf82e to 0273878 Compare May 7, 2019 17:47
@wolffcm
Copy link
Author

wolffcm commented May 7, 2019

As it turns out, it's not possible to use go test -race with an Alpine-based image:
golang/go#14481
Incidentally, go test -race also requires CGO, so we would need to include gcc and libc-dev dependencies anyway, but even with them there I get the link errors described in the golang issue.

I'll update the issue to reflect this, a different solution is needed.

@wolffcm wolffcm closed this May 7, 2019
@jacobmarble jacobmarble deleted the chore/udpate-ragel-ci-version branch January 4, 2024 16:45
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.

CI requires particular version of ragel to pass checkgenerate
2 participants