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 clang into ftl-build images + code maintainance #94

Merged
merged 9 commits into from
May 24, 2024
Merged

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented May 20, 2024

What does this implement/fix?

See title, containers are now based off of alpine:edge so we notice upcoming issues earlier.


Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): N/A


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

@DL6ER DL6ER requested a review from a team May 20, 2024 08:55
@dschaper
Copy link
Member

I'm not all that comfortable with using edge.

@DL6ER
Copy link
Member Author

DL6ER commented May 20, 2024

Verified to work by re-enabling pushes on branches. I honestly think we should have this. Not only to have branches available for testing while we are awaiting review (and hence don's unnecessarily slow down the process), but also as this would have prevented exactly what we see here. The con-argument is that it may clutter up https://hub.docker.com/r/pihole/ftl-build/tags but I honestly don't see this as ftl-build is really an image solely used internally in Pi-hole processes.

Screenshot from 2024-05-20 20-39-18
Screenshot from 2024-05-20 20-39-33

@DL6ER
Copy link
Member Author

DL6ER commented May 20, 2024

@dschaper The idea about using edge is to get the most recent compilers and libraries when building new build images. This will be especially useful in the nightly images where we'll see any upcoming issues early on and can report things upstream if needed.

@PromoFaux
Copy link
Member

I think, - and I might be wrong here - other thoughts are welcome! - using edge is fine for the FTL buld image. I wouldn't want to use it in the mainline pihole/pihole image.

Though for balance.. @DL6ER, besides what is already mentioned... is there anything we get from edge that we can't do with 3.19/latest?

@DL6ER
Copy link
Member Author

DL6ER commented May 21, 2024

Quoting Alpine:Edge`:

End users should not use "edge" as their main day-to-day workstation or as productive system. Because "edge" is a development branch, many changes are not heavily tested (or tested at all) and packages in "edge" can and sometimes do break without warning.

However, testing "edge" is a very valuable activity which helps the Alpine Linux development to ensure that the quality of the stable releases is high. Testing "edge" is a great way to contribute to the Alpine Linux development.

Having acknowledged the former, let's focus on the latter. There was a bug with LTO that was also there in v3.19. In edge we reported it and it was fixed two days after. I'd you all can sleep better with latest, I have no real objections changing it. My opinion is that with edge we are seeing stuff that breaks earlier and can intervene before it is released.

My plan is to eventually use nightly as local devcontainer. The tagged containers (what the CI uses) stay fixed to the time when we tag them. There is no frequent refreshing and our containers have to perform well in the entire lengthy CI verification as normal FTL build have to. If they succeed to compile and the binary checks are fine and the entire CI checking is green, I'm pretty sure the generated binary is fine.

@dschaper
Copy link
Member

Looking over the code here I see we already are using FROM alpine:edge as the base image, so the only thing that is changing is to use the edge version of powerdns instead of the specifically pinned versions of that package from the community repo?

I'm a bit confused here because I don't see any diff that shows that the base image for the docker images has changed.

image

@DL6ER
Copy link
Member Author

DL6ER commented May 21, 2024

It's here

Screenshot_2024-05-21-16-49-43-95_40deb401b9ffe8e1df2f1cc5ba480b12

@yubiuser
Copy link
Member

I also hesitate to build everything based on a rolling tag. But I see your point in noticing issues early.
We currently build for risc based on alpine edge - shouldn't this be enough to notice issues?

@DL6ER
Copy link
Member Author

DL6ER commented May 22, 2024

The architectures are independent and each can really have their own issues. How about we change the "normal" building to "latest" (stable) and the "nightly" (once a week) to "edge" using a suitable if in the workflow?

@yubiuser
Copy link
Member

This sounds like a good compromise.

@dschaper
Copy link
Member

I concur, the compromise sounds good to me as well.

@DL6ER
Copy link
Member Author

DL6ER commented May 22, 2024

Compromise implemented, now waiting on the containers to successfully build

Signed-off-by: DL6ER <[email protected]>
@dschaper
Copy link
Member

Can any of the images be built on the self-hosted runners? Some of these steps look like they take over 30 minutes to run.

@DL6ER
Copy link
Member Author

DL6ER commented May 24, 2024

Relying on the self-hosted builders may be a bottleneck here as we have so few. As building containers should only happen rarely - once per week scheduled and then every few months on manual changes - this seems acceptable.

@DL6ER DL6ER merged commit f575f4d into master May 24, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants