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

docker: update some dockerfiles #588

Closed
wants to merge 18 commits into from

Conversation

asarhaddon
Copy link
Contributor

The modified implementations have been tested on Debian bullseye.

  • prefer latest Ubuntu image, hopefully reducing the image count

  • for Ubuntu versions, prefer numerical versions to unsorted codenames

  • prefer packaged build systems to local rebuilds

  • prefer the current default version of each build system

  • delegate resolution of indirect dependencies to apt

The modified implementations have been tested on Debian bullseye.

* prefer latest Ubuntu image, hopefully reducing the image count

* for Ubuntu versions, prefer numerical versions to unsorted codenames

* prefer packaged build systems to local rebuilds

* prefer the current default version of each build system

* delegate resolution of indirect dependencies to apt
Copy link
Collaborator

@dubek dubek left a comment

Choose a reason for hiding this comment

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

Looks good. See one small comment below.

@asarhaddon have you built these new images and tests the mal impls in them? That's a lot of work...

[FROM ubuntu:vivid
[FROM ubuntu:20.04
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, what is this [ (also in the original file)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was apparently inoffensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am testing manual installations on a physical machine. This does not catch all issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From your comment and the error message with the factor language, I guess that Dockerfiles are not executed before each test run, but only used when manually generating a new image for each language.
If this is correct, the results are probably not meaningful, but the merge request at least documents a way to recreate a local test environment, and may be useful during next update of the image.

Spotted by dubek.
The bracket was apparently ignored.
The initial motivation is to provide an explicit path to the python3
interpreter, as the versioned executable is not always in PATH.
@asarhaddon
Copy link
Contributor Author

The xslt failures illustrate such differences. I have hopefully solved the first issue (python3 may not be in PATH), but now runtest.py fails to find impl/xslt/run on actual docker images. make test^xslt works on my machine. Any idea?

Debian/Ubuntu do not ship a python->python2 link by default.

With the improved logo implementation in another branch, there is no
need anymore to tweat the garbage collector settings.
@asarhaddon
Copy link
Contributor Author

Hello.
While working on #592, I have modified the logo implementation. It now passes tests for step5 with the default garbage collection settings, at least on my machine.
The choice to download the source from Debian instead of upstream reflects my own habits (GPG signature for the downloaded contents, daily tests ensuring that the current version builds with latest toolchains, and so on). I have of course no strong opinion about that.
The Debian package is usually based on the contents of the upstream tarball, including ./configure. I could have avoided the dependency on autoconf, but the other Dockerfiles gave me the impression that keeping the images minimal was not a priority.

@dubek
Copy link
Collaborator

dubek commented Jan 13, 2022

Sure.

Explictly select python 3.  The `python2` and `python` packages will
be removed from Ubuntu.  Until each call site is fixed, install a
/usr/local/bin/python symbolic link as a non-intrusive work-around
(.deb packages do not interfer with /usr/local).

Add an explicit maintainer for bbc-basic.

Undo some cosmetic changes in order to reduce the global diff, this
merge request will probably be sqashed before acceptance.

Move lib{readline,edit}-dev out of the generic part.

Use existing .deb packages for GHDL and vim.
@kanaka
Copy link
Owner

kanaka commented Aug 5, 2024

@asarhaddon BTW, so that we don't duplicate work, I'm working on getting this refactored on this personal branch: https://github.com/kanaka/mal/tree/update-some-dockerfiles. I think I just need to track down a go build issue and then I'll merge since it will be no worse (i.e. elm, common-lisp, and objpascal that have issues).

@kanaka
Copy link
Owner

kanaka commented Aug 5, 2024

I have refactored these and merged them to master now. Here is the CI workflow: https://github.com/kanaka/mal/actions/runs/10256887753 Should just be objpascal, elm, and common-lisp that fail.

@kanaka kanaka closed this Aug 5, 2024
@asarhaddon asarhaddon deleted the update-some-dockerfiles branch August 6, 2024 04:43
@asarhaddon
Copy link
Contributor Author

In order to fix objpascal, just remove the CMem module from the list of imports at the beginning of each step file.
The common-lisp failure changes when the Dockerfile installs the cl-ppcre Debian package but other errors happen before the tests begin. By the way, there also exists a package named cl-quicklisp, but the user must install manually (https://salsa.debian.org/common-lisp-team/quicklisp/-/blob/master/debian/README.Debian?ref_type=heads).

@kanaka
Copy link
Owner

kanaka commented Aug 6, 2024

@asarhaddon Removing the CMem module did the trick. Out of curiosity, where did you find the info about CMem?

@kanaka
Copy link
Owner

kanaka commented Aug 6, 2024

@asarhaddon The common-lisp issue is weird. If I go into the container and run make 5 times, each time it complains about a different dependency and then eventually succeeds. So something isn't quite hooked up right to work with more recent versions of stuff.

@asarhaddon
Copy link
Contributor Author

I have found https://wiki.freepascal.org/CMem while investigating the differences between step0 (not affected) and step1 (affected). Now, CMem is present in both, and motivated by the C pointers in readline, so trying to remove it was a short in the dark.

@kanaka
Copy link
Owner

kanaka commented Aug 6, 2024

@asarhaddon sigh, I got common-lisp working on my local system by using sbcl to build instead of cl-launch: 062fb6f.

However, in GHA CI, it crashes right after building the first step: https://github.com/kanaka/mal/actions/runs/10270914448/job/28419731317

I've filed a ticket and pinged the original author: #660

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.

3 participants