Use Dockerfile and script for building in container instead of running the entire job in the container #34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I thought the action name could be anything, but apparently it has to be action.yml. I created an actions folder just to make it clearer that this is an action, not a workflow
Used cd in a weird way but if it works it works
Few mistakes I made while porting over the code
Need it for the snapshot and publish outside the docker container
…build finishes Caused by gradle files being left behind by the docker container. Having to install gradle two times for the linux job is a bit unclean, though. Also added no-daemon to the gradle task used because only one gradle command is being executed in the docker container.
See https://github.com/koalaman/shellcheck/wiki/SC2164 for why this is good practice.
Using apt-get because apparently apt does not have a stable CLI interface. While apt install is probably safe, using apt-get will prevent the warnings in the log at least. See https://askubuntu.com/a/990838 As for sudo, while it's not needed sudo also doesn't cause any harm and those commands technically do need root permissions and using sudo makes that clear.
Seems the zulu package I'm adding pulls in x11 which is unnecessary as we are running this in a headless setting.
Can't use sudo as the start as it needs to be installed first... so I just decided to not use sudo at all if it's not needed. But I'll keep the package install of sudo in case some other command needs it
We're installing zulu, so I don't think this is needed.
Hopefully this fixes the "Unable to delete directory build/generated/sources/annotationProcessor/java/main" error Not using a daemon still because it's only two tasks.
Probably needed for the deploy to work properly. Honestly maybe I could move the deploy into the docker container but thing is it'd require passing in the secrets
I used https://docs.github.com/en/actions/sharing-automations/creating-actions/creating-a-docker-container-action#creating-an-action-metadata-file as a starting point, but don't need these inputs or outputs.
|
How it works is that when it enters the docker container, the working directory is mounted and mapped from the runner onto the container. So the runner does all the GitHub actions to clone/setup the repo and then enters the container to build the natives and snapshot. After the container exits the changes persist on the runner and the runner uploads the artifacts and deploys the snapshot. |
SonicGDX
changed the title
No longer doing it in multiple steps, so adding it to GITHUB_ENV won't really do anything, if it even works in the docker container right now which it probably doesn't.
Hopefully should prevent java 11 JRE from being installed, as zulu should provide what maven and ant needs. Also specified default find path as recommended by https://github.com/koalaman/shellcheck/wiki/SC2185
The log seems to be massive for the docker step so hopefully this reduces it a bit.
Strangely I got an error while trying to download an artifact for no reason. So maybe updating actions could help? Either way we should be good to do this now that the job is no longer being run in a docker container with glibc 2.17 May need to audit these actions though.
The new setup-gradle seems to validate the wrappers, but strangely the wrappers from the sdl submodule fail to verify for some reason.
From TheOfficialGman's response to my questions here: libgdx#23 (comment)
I think the main issue is still just verifying that publish works properly. Unfortunately publish still seems to build stuff in the runner even when the build command was run inside the container. Hopefully it doesn't cause any issues though. Also added some comments I missed when porting the action steps to the script
…dering for apt-get apt-get update is pretty noisy by default as there are a lot of lines for it reading the database. This should make it a bit quieter I also noticed in the later part of the script -yq was before the install/update command so I made that consistent as well.
If not deploying the snapshot, there is no benefit to setting up JDK or Gradle, so we might as well skip those steps too to save a bit of time.
When I inspected the natives of a build from my branch and compared it to master, I noticed that while the master output-libs artifact only had one jar file in it (jamepad-2.30.0.0-SNAPSHOT-natives-desktop.jar) mine had 4 files in them (jamepad-2.30.0.0-SNAPSHOT.jar alongside other files with "workspace" at the start instead of jamepad for some reason, including the one with natives-desktop at the end). I figure this is because the build was being done before the deploy. As for the workspace name, it may be to do with how the container is configured...
It's only one gradle command being run in the job, so the daemon probably isn't needed.
|
Sorry for so many unnecessary CI runs, I really should be pushing less often lol. But I think I've mostly finished this now. Since @theofficialgman developed the previous docker setup in #23 I think it would be very helpful if they reviewed this PR (if possible). |
SonicGDX
commented
Dec 3, 2024
SonicGDX
added a commit
to SonicGDX/libgdx
that referenced
this pull request
Dec 15, 2024
- Used apt-get instead of apt because using apt in a script gives the warning message "apt does not have a stable interface". - Removed usages of sudo just to be consistent (I can't always use sudo because initially sudo isn't installed) - Added --no-daemon to gradle command because only one gradle command is being run so the daemon isn't necessary. - Added --quiet to apt-get usages to cut down unuseful lines in logs Having the action be in a separate actions folder may not be desirable, in which case I can probably move the action into the workflow folder somewhere but I'm not sure where. Copy my docker action from my similar PR in Jamepad Copied from libgdx/Jamepad#34 Similar structure should work, but will of course need to adjust to suit the libgdx action
|
Whoops, almost lost some of my work due to an accidental force push. Thankfully I still had it on my other machine. Force pushing is very dangerous, kids! |
SonicGDX
added a commit
to SonicGDX/libgdx
that referenced
this pull request
Dec 15, 2024
I've changed the setup of the natives-linux job so that hopefully it continues to work with newer versions of upload-artifact. I've done this by moving the docker setup into its own action (which is local to the repo) that runs a script in the container that builds the natives and snapshots, but doesn't use any third party actions - meaning it should not be need node 20 to be used in the container. Other minor changes: - Used apt-get instead of apt because using apt in a script gives the warning message "apt does not have a stable interface". - Removed usages of sudo just to be consistent (I can't always use sudo because initially sudo isn't installed) - Added --no-daemon to gradle command because only one gradle command is being run so the daemon isn't necessary. - Added --quiet to apt-get usages to cut down unuseful lines in logs - Moved around arguments to apt-get so they're always at the front - removed setup-java and gradle-build-action usages in the natives-linux step because zulu installation is done in the container and gradle is downloaded when the wrapper is run. Having the action be in a separate actions folder may not be desirable, in which case I can probably move the action into the workflow folder somewhere but I'm not sure where. Copied docker action structure from my similar PR in Jamepad (libgdx/Jamepad#34)
SonicGDX
added a commit
to SonicGDX/libgdx
that referenced
this pull request
Dec 15, 2024
I've changed the setup of the natives-linux job so that hopefully it continues to work with newer versions of upload-artifact. I've done this by moving the docker setup into its own action (which is local to the repo) that runs a script in the container that builds the natives and snapshots, but doesn't use any third party actions - meaning it should not be need node 20 to be used in the container. Other minor changes: - Used apt-get instead of apt because using apt in a script gives the warning message "apt does not have a stable interface". - Removed usages of sudo just to be consistent (I can't always use sudo because initially sudo isn't installed) - Added --no-daemon to gradle command because only one gradle command is being run so the daemon isn't necessary. - Added --quiet to apt-get usages to cut down unuseful lines in logs - Moved around arguments to apt-get so they're always at the front - removed setup-java and gradle-build-action usages in the natives-linux step because zulu installation is done in the container and gradle is downloaded when the wrapper is run. Having the action be in a separate actions folder may not be desirable, in which case I can probably move the action into the workflow folder somewhere but I'm not sure where. Copied docker action structure from my similar PR in Jamepad (libgdx/Jamepad#34)
SonicGDX
added a commit
to SonicGDX/libgdx
that referenced
this pull request
Dec 15, 2024
I've changed the setup of the natives-linux job so that hopefully it continues to work with newer versions of checkout and upload-artifact. I've done this by moving the docker setup into its own action (which is local to the repo) that runs a script in the container that builds the natives and snapshots, but doesn't use any third party actions - meaning it should not be need node 20 to be used in the container. Other minor changes: - Used apt-get instead of apt because using apt in a script gives the warning message "apt does not have a stable interface". - Removed usages of sudo just to be consistent (I can't always use sudo because initially sudo isn't installed) - Added --no-daemon to gradle command because only one gradle command is being run so the daemon isn't necessary. - Added --quiet to apt-get usages to cut down unuseful lines in logs - Moved around arguments to apt-get so they're always at the front - removed setup-java and gradle-build-action usages in the natives-linux step because zulu installation is done in the container and gradle is downloaded when the wrapper is run. Having the action be in a separate actions folder may not be desirable, in which case I can probably move the action into the workflow folder somewhere but I'm not sure where. Copied docker action structure from my similar PR in Jamepad (libgdx/Jamepad#34)
SonicGDX
added a commit
to SonicGDX/libgdx
that referenced
this pull request
Dec 15, 2024
I've changed the setup of the natives-linux job to not require needing Node 20, so hopefully it will continue to work with newer versions of checkout and upload-artifact for the forseeable future. I've done this by moving the docker setup into its own action (which is local to the repo) that runs a script in the container that builds the natives and snapshots, but doesn't use any third party actions - meaning it should not be need node 20 to be used in the container. Other minor changes: - Used apt-get instead of apt because using apt in a script gives the warning message "apt does not have a stable interface". - Removed usages of sudo just to be consistent (I can't always use sudo because initially sudo isn't installed) - Added --no-daemon to gradle command because only one gradle command is being run so the daemon isn't necessary. - Added --quiet to apt-get usages to cut down unuseful lines in logs - Moved around arguments to apt-get so they're always at the front - removed setup-java and gradle-build-action usages in the natives-linux step because zulu installation is done in the container and gradle is downloaded when the wrapper is run. Having the action be in a separate actions folder may not be desirable, in which case I can probably move the action into the workflow folder somewhere but I'm not sure where. Copied docker action structure from my similar PR in Jamepad (libgdx/Jamepad#34)
SonicGDX
added a commit
to SonicGDX/libgdx
that referenced
this pull request
Dec 15, 2024
I've changed the setup of the natives-linux job to not require needing Node 20, so hopefully it will continue to work with newer versions of checkout and upload-artifact for the forseeable future. I've done this by moving the docker setup into its own action (which is local to the repo) that runs a script in the container that builds the natives and snapshots, but doesn't use any third party actions - meaning it should not be need node 20 to be used in the container. Other minor changes: - Used apt-get instead of apt because using apt in a script gives the warning message "apt does not have a stable interface". - Removed usages of sudo just to be consistent (I can't always use sudo because initially sudo isn't installed) - Added --no-daemon to gradle command because only one gradle command is being run so the daemon isn't necessary. - Added --quiet to apt-get usages to cut down unuseful lines in logs - Moved around arguments to apt-get so they're always at the front - removed setup-java and gradle-build-action usages in the natives-linux step because zulu installation is done in the container and gradle is downloaded when the wrapper is run. Having the action be in a separate actions folder may not be desirable, in which case I can probably move the action into the workflow folder somewhere but I'm not sure where. Copied docker action structure from my similar PR in Jamepad (libgdx/Jamepad#34)
SonicGDX
added a commit
to SonicGDX/libgdx
that referenced
this pull request
Dec 15, 2024
I've changed the setup of the natives-linux job to not require needing Node 20, so hopefully it will continue to work with newer versions of checkout and upload-artifact for the forseeable future. I've done this by moving the docker setup into its own action (which is local to the repo) that runs a script in the container that builds the natives and snapshots, but doesn't use any third party actions - meaning it should not be need node 20 to be used in the container. Other minor changes: - Used apt-get instead of apt because using apt in a script gives the warning message "apt does not have a stable interface". - Removed usages of sudo just to be consistent (I can't always use sudo because initially sudo isn't installed) - Added --no-daemon to gradle command because only one gradle command is being run so the daemon isn't necessary. - Added --quiet to apt-get usages to cut down unuseful lines in logs - Moved around arguments to apt-get so they're always at the front - removed setup-java and gradle-build-action usages in the natives-linux step because zulu installation is done in the container and gradle is downloaded when the wrapper is run. - gnupg gets installed anyway as a dependency but I added it explicitly because it is needed for dearmoring the azul key Having the action be in a separate actions folder may not be desirable, in which case I can probably move the action into the workflow folder somewhere but I'm not sure where. Copied docker action structure from my similar PR in Jamepad (libgdx/Jamepad#34)
Tom-Ski
pushed a commit
to libgdx/libgdx
that referenced
this pull request
Dec 16, 2024
I've changed the setup of the natives-linux job to not require needing Node 20, so hopefully it will continue to work with newer versions of checkout and upload-artifact for the forseeable future. I've done this by moving the docker setup into its own action (which is local to the repo) that runs a script in the container that builds the natives and snapshots, but doesn't use any third party actions - meaning it should not be need node 20 to be used in the container. Other minor changes: - Used apt-get instead of apt because using apt in a script gives the warning message "apt does not have a stable interface". - Removed usages of sudo just to be consistent (I can't always use sudo because initially sudo isn't installed) - Added --no-daemon to gradle command because only one gradle command is being run so the daemon isn't necessary. - Added --quiet to apt-get usages to cut down unuseful lines in logs - Moved around arguments to apt-get so they're always at the front - removed setup-java and gradle-build-action usages in the natives-linux step because zulu installation is done in the container and gradle is downloaded when the wrapper is run. - gnupg gets installed anyway as a dependency but I added it explicitly because it is needed for dearmoring the azul key Having the action be in a separate actions folder may not be desirable, in which case I can probably move the action into the workflow folder somewhere but I'm not sure where. Copied docker action structure from my similar PR in Jamepad (libgdx/Jamepad#34)
|
Is this ready to merge since the pr to the main repo was merged? |
Not right now, but after I fix the jar names in the output being wrong it should be. I think using EDIT: |
Main libgdx repo uses v3 of setup-java and upload/download artifact so I thought it didn't make sense to use v4 of those here. Similarly it uses ubuntu 20.04. Keeping checkout at v4 because it is currently being used in this repo.
…tead of Jamepad-x.x.x.x-SNAPSHOT-natives-desktop.jar
|
It should be ready to merge now. Though I did make a ton of commits, so you'd probably want to merge with Squash and Merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As I mentioned in #23 and in libgdx/libgdx#7477, the upload-artifact and download-artifact@v3 versions will be deprecated on November 30th. As v4 and v5 require node 20, when this happens it will break the existing linux job that uses a docker container for the whole job as the container uses glibc 2.17 which doesn't work with node 20.
This isn't fully ready yet, but I've changed the setup so that hopefully it continues to work with newer versions of actions like upload-artifact and setup-java. I've done this by moving the docker setup into its own action (which is local to the repo) that runs a script in the container that builds the natives and snapshots, but doesn't use any third party actions - meaning it should not be need node 20 to be used in the container.
Inspired by https://docs.github.com/en/actions/sharing-automations/creating-actions/creating-a-docker-container-action#introduction
The action does seem to run properly but of course I haven't tested it fully and there could be other issues with it.