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

GUACAMOLE-1374: Guacamole in Docker on ARM #499

Merged
merged 1 commit into from
Apr 7, 2024
Merged

GUACAMOLE-1374: Guacamole in Docker on ARM #499

merged 1 commit into from
Apr 7, 2024

Conversation

Nbelles
Copy link
Contributor

@Nbelles Nbelles commented Mar 23, 2024

Pull request based on discussion here.

It is difficult to dynamically determine build options based on a given architecture from within a Dockerfile without requiring manually passing in the architecture type as an argument during the build process. Instead, this proposed approach uses the fact that a build script is used to actually build the dependencies and checks the architecture there. With this information it generates a list of overrides which are appended to the end of the install_from_git functions. Options are processed in order so by appending them to the end, they will override any options provided in the main Dockerfile. This guarantees the build process succeeds (even if the user provides bad options in the Dockerfile).

I have tested that it works on macOS (arch: amd64) and a Raspberry Pi 4 (arch: aarch64), open to hearing how it works on other architectures!

@necouchman
Copy link
Contributor

@Nbelles Thanks for putting this in. All pull requests and commits need to be associated and tagged with a Jira issue. In this case, you're in luck, because one already exists to cover this:

https://issues.apache.org/jira/browse/GUACAMOLE-1374.

If you could tag both the PR and your commit messages with "GUACAMOLE-1374:", that would be great.

@Nbelles Nbelles changed the title Guacamole in Docker on ARM GUACAMOLE-1374: Guacamole in Docker on ARM Mar 24, 2024
@Nbelles
Copy link
Contributor Author

Nbelles commented Mar 24, 2024

I think that should do the trick. I will also create a separate pull request shortly for the guacamole-client changes that need to be made to make that work on ARM as well.

I think the next step towards providing ARM Docker images on Docker Hub would be to follow something like the direction provided by this Docker guide. I'm not sure how the Docker Hub build process for this repo is set up right now (I'm only seeing the code for building on pull request) but it seems like the next step would be to adjust the docker build command to be a docker buildx build similar to that guide to include building for multiple architectures.

@necouchman
Copy link
Contributor

@Nbelles So you confirmed that the build behaves as expected when you specify both -DWITH_SSE2=ON -DWITH_SSE2=OFF, as the OFF one occurs last?

@Nbelles
Copy link
Contributor Author

Nbelles commented Apr 2, 2024

I have verified that the build process completes on ARM when the OFF flag comes after the ON flag (when just the ON flag fails to build on ARM).

I have someone visiting me tomorrow that has a Windows computer that I will be able to verify that RDP still works as expected when those flags are set according to the commit. Will report back with those results late tomorrow evening.

@necouchman
Copy link
Contributor

What about this:

https://nielscautaerts.xyz/making-dockerfiles-architecture-independent.html

In particular, using the TARGETPLATFORM build variable:

https://docs.docker.com/reference/dockerfile/#automatic-platform-args-in-the-global-scope

It seems like this would allow for actually conditionally setting the FREERDP_OPTS variable inside the Dockerfile, rather than resorting to the extra variable in the build script?

@Nbelles
Copy link
Contributor Author

Nbelles commented Apr 3, 2024

I had looked into this methodology and I like the idea of doing the logic directly in the .Dockerfile but the downside to this approach is that I don't see a way for a user to easily build it themselves without forcing them to determine their target platform and specify it to the docker buildx build ... command.

One route would be to use the TARGETPLATFORM argument which then forces all users who want to build the container themselves to use the docker buildx build ... command to build the .Dockerfile. The build command that pushes images to the docker hub would have to be updated as well. This would also mean the documentation would need to get updated to include instructions on how to build with docker buildx build ... because I do not believe that is common knowledge, as compared to the normal docker build ... command. We could also create a short script that determines the target platform and passes that to the docker buildx build ... command to make it easier for users to use this method but I think this is more work than the method used in the above commit.

The other route (as prescribed in the above commit) would be to override any erroneous build options that the platform doesn't support. Then a user could put desired options (potentially incorrectly) in the .Dockerfile and the build-all.sh file would account for any discrepancies between what is selected and what is required for the platform. We could add some additional logic to check the variables and print some warning about any inconsistent build options that are being overridden if that helps.

I personally prefer the latter option so that the build process is easy for users (albeit slightly more complex on the backend) but I'm open to hearing what others think about this tradeoff.

Note: The reason I think being able to build the docker image yourself is so important is that for anyone who develops their own extensions and wants to run them in docker (such as myself), there is currently no easy way to build the jar file and load it into the docker image with guacamole properties passed through without building the image yourself. I hope to create another pull request in the near future that allows for easy jar file and guacamole properties loading using the docker hub provided images.

(I still plan to verify the provided commit works as expected shortly)

@Nbelles
Copy link
Contributor Author

Nbelles commented Apr 3, 2024

I just confirmed that I was able to connect to a Windows machine with RDP successfully through guacamole 1.5.5 on ARM on a Raspberry Pi 5 with the suggested commit.

@necouchman
Copy link
Contributor

@Nbelles Thanks for that detailed info.

@mike-jumper or @jmuehlner Thoughts on this approach to modifying flags for the Docker build?

# Determine any option overrides to guarantee successful build
#

export BUILD_ARCHITECTURE="$(arch)" # Determine architechutre building on
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: architechutre


case $BUILD_ARCHITECTURE in
armv6l|armv7l|aarch64)
echo "Using ARM overrides"
Copy link
Contributor

Choose a reason for hiding this comment

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

These echo statements look useful for debugging this change, but not so useful for general usage. If you look at above logging examples like "Building $REPO_DIR @ $VERSION ...", it includes useful information that varies from build to build, where as "Using ARM overrides" is going to appear verbatim in the build output of every single ARM build ever, and also doesn't add any additional information that the output on line 103 isn't already telling you.

I'd just remove these last 2 log lines entirely.

@jmuehlner
Copy link
Contributor

@Nbelles Thanks for that detailed info.

@mike-jumper or @jmuehlner Thoughts on this approach to modifying flags for the Docker build?

I don't have an issue with this approach.

@Nbelles
Copy link
Contributor Author

Nbelles commented Apr 5, 2024

@jmuehlner Thanks for catching the spelling error and I removed the extra print statements in the latest commit.

@jmuehlner
Copy link
Contributor

@jmuehlner Thanks for catching the spelling error and I removed the extra print statements in the latest commit.

Thanks for the cleanup! Note that your second commit message still needs a GUACAMOLE-1374 prefix, or alternatively you might consider just squashing it into the first commit.

@Nbelles
Copy link
Contributor Author

Nbelles commented Apr 7, 2024

@jmuehlner Thanks for reminding me, should be squashed.

@necouchman
Copy link
Contributor

@jmuehlner Good with it if you are...

@jmuehlner
Copy link
Contributor

@jmuehlner Good with it if you are...

Looks good to me.

@necouchman necouchman merged commit 633d5b9 into apache:main Apr 7, 2024
1 check passed
@Nbelles Nbelles deleted the feature/docker-build-on-arm branch April 10, 2024 00:32
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