Skip to content

Cross platform support #57

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

Merged
merged 16 commits into from
Sep 14, 2022

Conversation

ImogenBits
Copy link
Collaborator

addressing #50 this PR adds CI checks for windows and macOS. It also includes 2 very minor bugfixes that I noticed because of the more extensive testing.

There were two big problems when making this that will also impact future work:

  1. The github macOS VMs don't come with a docker daemon preinstalled, apparently because of licensing issues. This means that we need to install it ourselves every time tests are run, which takes about ten minutes. I'm working on another PR that adds more tests that will also include tests for the whole battle script, so we'll then only have to install it once.
  2. The github windows VMs have the docker daemon run in windows container mode, apparently due to VM nesting issues. Because of that we can't run any linux images in our tests. I think the "proper" solution here would be to have multiple dockerfiles for each test and have the testing framework choose the right one based on the os. But since our testing docker images are so simple (either doing a no op, raising an error, or sleeping for a long time) I decided to just make all dockerfiles work on both OSes. Because of that we need to use the shell form of the entrypoint (ENTRYPOINT sleep 10 rather than ENTRYPOINT ["sleep", "10"]) and alias a windows image to 'alpine'.

Currently the tests running the whole script fail on macOS. From what I can tell this is not because of an actual OS issue or bug in our code but because installing scripts works differently than on linux/windows. I have no experience with that and can't find anything related to that on google so I'm just leaving it as is.

@Benezivas
Copy link
Collaborator

Thank you for adressing issue #50 with this PR. I advocate for formulating the README.md statement a bit more cautiously, as neither of us seems to have access to a macOS machine in order to properly test the code on this OS. I would advocate leaving the CI for macOS be until we have figured out how to properly implement it, so that we do not carry along a failing CI pipeline in this release candidate. We could exclude the run of the actual script on macOS and remark this in issue #50 for someone with access to an apple device to fix.

@ImogenBits
Copy link
Collaborator Author

yeah sounds smart, implemented both changes

@Benezivas Benezivas self-assigned this Sep 14, 2022
@Benezivas Benezivas merged commit 22583ac into Algorithmic-Battle:4.0.0-rc Sep 14, 2022
@ImogenBits ImogenBits deleted the cross_platform branch September 14, 2022 09:19
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