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

Update ubuntu64 Dockerfile to Focal Fossa (20.04 LTS) and LLVM 10 #68

Merged

Conversation

straight-shoota
Copy link
Member

Ubuntu has dropped i386 architecture in 19.10, so there won't be a 20.04 LTS release for i386.

Also adds a separate Dockerfile for i386 to maintain them separately.

@bcardiff
Copy link
Member

Isn't it a bit too soon to use focal in official docker images?

@straight-shoota
Copy link
Member Author

Why would it be too soon? Focal is released and seems to be stable.

@bcardiff
Copy link
Member

It was released 1 month ago. These images should be widely used. There are no compatibility issues that users or libraries are facing?

@straight-shoota
Copy link
Member Author

There are no compatibility issues that users or libraries are facing?

I'm not aware of any. By now, there should have been reports on that.
The libraries in Ubuntu are still somewhat conservative and we've been using these versions for a long time in other distributions. So I wouldn't expect much friction here.

@petr-fischer
Copy link

+1 - get rid of LLVM 8.0.0.

Our case: https://forum.crystal-lang.org/t/invalid-memory-access-in-crystal-compiler-pre-solved/2237/10

(of course, I understand that our team is not the only one in the world)

@bcardiff
Copy link
Member

bcardiff commented Aug 5, 2020

@petr-fischer this PR would not get rid of LLVM 8. It will still be used on the .tar.gz / .deb / .rpm since that depends on https://github.com/crystal-lang/distribution-scripts/blob/master/linux/Dockerfile#L46

This will only affect the LLVM used to run the compiler specs in the CI. So the regular failures on the dist steps will still happen.

We would need to update to alpine v3.12 to use llvm-10, yet I am not sure if 10.0.0 should be used. I recall some issues in 10.0.0 regarding debug metadata lang identifier, but I might be wrong. (update: the issue was crystal-lang/crystal#9148 (comment), and it should not affect since we are using cpp identifier currently)

@bcardiff
Copy link
Member

bcardiff commented Aug 5, 2020

@straight-shoota do you want to bump alpine and llvm here and we can check if it works in the usual way?

@straight-shoota
Copy link
Member Author

Looks good 👍

docker/alpine.Dockerfile Outdated Show resolved Hide resolved
@bcardiff
Copy link
Member

bcardiff commented Aug 7, 2020

There are a couple of issues I am checking to see that the CI would not break with this.

The update to alpine and llvm done here does not change the llvm used in the .tar.gz / .dep / .rpm files

I am applying those changes here 4edec88...983a156 and validating how things work.

To recap

  • the -build images (ubuntu and alpine) need a llvm installed to build the compiler. These are built by ./docker/*
  • the .tar.gz file are built by ./linux/* files (these were not updated)
  • I already notice that curl and libz need to be added in a couple of places

I was also checking if the llvm@10 bump can be applied to darwin.

Once I verified things are able to work I will push some commits here and merge the PR if that sounds ok to you @straight-shoota

@bcardiff
Copy link
Member

I needed to rebase on master to correctly apply the last 4 commits @straight-shoota .
The commits are self explanatory I think.

It's a pitty we are with llvm-8 on the 32 bits build-images only, yet we are able to package 32 bits with llvm 10.

All this make me want to change how the CI and packing is done since it's clearly a source of confusion. Maybe in the future we can clean up dependencies by switching to Nix. This will also come handy on osx actually. But is for a future iteration.

I've checked the CI already with these changes and we are good to go I think.

Please, squash merge if you agree with my last changes.

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.

4 participants