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

Dockerfile for grounder. #7

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rpgoldman
Copy link

No description provided.

@rpgoldman
Copy link
Author

I have marked this as a WIP because to run it I have been also using a script file that I believe to be poorly designed.

@rpgoldman
Copy link
Author

Also, if I knew more about GitHub actions, I would suggest setting up a GitHub action that would automatically build the Docker image.

@rpgoldman rpgoldman mentioned this pull request Jul 26, 2023
@galvusdamor
Copy link
Collaborator

True! That is something, I could try to figure out at some point :)

Take the standard GitHub action for building dockerfiles and adapt it for building generic PANDA subsystems (so that it could be copied to other repositories).
@rpgoldman
Copy link
Author

For simple cases like this, there is a sample GitHub action, which I have attempted to insert into the branch. Not sure if it will run -- may need to extend the on: part to make it testable.

@rpgoldman
Copy link
Author

I'm not sure how to test the GitHub action -- I added the workflow_dispatch setting, which should allow the workflow to be manually triggered for testing. But... according to the docs, you can only manually trigger workflows that are on the main branch (in our case master).

@galvusdamor
Copy link
Collaborator

For your copy there is really no reason to keep the changes in a separate branch (as we still have the panda-planner-dev:master as the overall true master). If you merge the dockerfile branch into your master, you can still PR your master into this one and try to test-run the dockerfile in your repo.

Docker image names must be lower-case, so can't use CaMelCAsE as in
the name of the binaries.
@rpgoldman
Copy link
Author

I have the dockerfile working again. I don't like the need to apply the patch, though, because it means that if I run docker build in a directory in which the patch has already been applied, then the build will fail.

That's why the Dockerfile now makes sure that cpddl is clean and then applies the patch.

This is very kludgy and will fail as soon as the patch becomes no longer necessary.

It would be much better to fork the cpddl repo, apply the patch to the fork, and use the fork for the external, instead of using upstream as the external and having to patch it. Doing so makes the build process ugly and brittle.

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.

2 participants