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

Explore adding native windows and linux driver #79

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

solvingj
Copy link

@solvingj solvingj commented Sep 3, 2024

This was mostly an exercise to familiarize myself with the codebase and test suite.

  • Learned about the existing native driver implementation
  • Initial attempt to refactor to allow for platform-specific native drivers
  • Create initial windows driver by copy/paste/modify of existing mac-centric driver
  • Create initial unit test of windows driver
  • Unit test and the fish.exe runs successfully on windows, printing desired messages
  • Copied/pasted folder to "linux" just for visual and discussion, no mod/testing

Stopping for discussion with @sparshev because it seemed likely to me the intention was to have the single existing Native driver service all platforms with conditionals around platform-specific behavior, and not using any abstractions like polymorphism. Need to ask and confirm, and if/so, would like specific guidance on how to handle the awkward implications in the native drivers Config.go, Driver.go, without the code becoming very bloated and hard to reason about.

This work also raised questions about how we might implement the configuration of the native driver to perform a limited subset of the current behavior, like "don't create new user', or alternative behaviors like "git clean workspace" and then "reboot after use". The goal here would be to implement native driver which just adds precise new features to our existing bare metal without changing anything fundamental like creating new users/etc.

Related Issue

#78

Motivation and Context

Add windows support for the Native Driver

How Has This Been Tested?

Testing incomplete, thus far, only exploratory work

  • Create initial unit test of windows driver
  • Unit test and the fish.exe runs successfully on windows, printing desired messages

Screenshots (if appropriate):

Here's output of new fish_native_windows_test.go unit test on windows:

GOROOT=C:\tools\go-1.23.0\go #gosetup
GOPATH=C:\tools\go-1.23.0\go\bin #gosetup
C:\tools\go-1.23.0\go\bin\go.exe test -c -o C:\Users\jwiltse\AppData\Local\JetBrains\GoLand2024.2\tmp\GoLand\___fish_native_windows_test_go.test.exe github.com/adobe/aquarium-fish/tests/drivers #gosetup
C:\tools\go-1.23.0\go\bin\go.exe tool test2json -t C:\Users\jwiltse\AppData\Local\JetBrains\GoLand2024.2\tmp\GoLand\___fish_native_windows_test_go.test.exe -test.v=test2json -test.paniconexit0 -test.run ^\QTest_fish_native_win\E$ #gosetup
=== RUN   Test_fish_native_win
=== PAUSE Test_fish_native_win
=== CONT  Test_fish_native_win
2024/09/05 18:21:23 INFO:	NativeWin: Created user for Application execution: fish-pzdvqr
2024/09/05 18:21:23 INFO:	NativeWin: The images are processed.
2024/09/05 18:21:27 INFO:	NativeWin: Started environment for user "fish-pzdvqr"
--- PASS: Test_fish_native_win (4.05s)
PASS

Process finished with the exit code 0

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@solvingj solvingj requested a review from sparshev as a code owner September 3, 2024 21:02
@solvingj
Copy link
Author

solvingj commented Sep 5, 2024

After discussion with @sparshev:

  • eliminated linux files, moved mac and win files into parent dir
  • renamed plat specific files with _plat suffix
  • added go:build platform pragmas to appropriate files
  • clarified some todos

New questions for @sparshev :

  • Do we want the unit tests for the native driver or not?
  • If so, do we add windows and macos test stages to the CI?
  • Can we add a boolean option for enable/disable the drivers "create user" step

Copy link
Collaborator

@sparshev sparshev left a comment

Choose a reason for hiding this comment

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

Just a bit of review to give some food for thought, later when structure will fit will do review properly.

@@ -1,3 +1,5 @@
//go:build darwin
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copyright should be at the top when possible, so better to move it after the copyright - those will work anywhere in the code like here:

* governing permissions and limitations under the License.
*/
//go:generate oapi-codegen -config types.cfg.yaml ../../docs/openapi.yaml
//go:generate oapi-codegen -config meta_v1.cfg.yaml ../../docs/openapi.yaml
//go:generate oapi-codegen -config api_v1.cfg.yaml ../../docs/openapi.yaml
//go:generate oapi-codegen -config spec.cfg.yaml ../../docs/openapi.yaml

… data and behavior using the standard mechanisms of go build pragmas, embedded structs, and shared functions
@solvingj solvingj force-pushed the feature/add_native_win_lin branch from a8ffe1b to b77590b Compare September 7, 2024 20:18
@solvingj
Copy link
Author

solvingj commented Sep 7, 2024

Ok i've re-implemented using embedded structs and go build pragmas, and some shared functions. Theres a bunch of error handling and return-value cleanup that still needs to happen, but submitting this now to make sure it matches @sparshev goal structurally.

@sparshev
Copy link
Collaborator

sparshev commented Sep 7, 2024

Yep, structurally looks very nice)

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