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

AB#2173: Add feature to bundle a signed executable with the current EGo runtime #144

Merged
merged 2 commits into from
Jun 29, 2022

Conversation

Nirusu
Copy link
Contributor

@Nirusu Nirusu commented Jun 21, 2022

Proposed changes

  • Add a new binary called 'ego-bundle', which is a self-extractable runtime
  • Add 'ego bundle' CLI command which bundles a signed EGo executable into the 'ego-bundle' binary and create a copy of it
  • Unit tests for both of them (including some host I/O, but I tried to limit it to where its necessary)

Additional info

For this PR I am not sure where to draw the line for code reuse. Many parts, especially for the unit tests are copied from the CLI or main EGo package to avoid calling NewCLI, though it is essentially the same code. On the other hand, I also was not sure whether to create an internal package to re-use functions, given that the bundle executable is supposed to pretty much stand on its own... I don't know, let me know in the review about your opinions.

Also don't be scared about the # of lines added, there's more test code involved than the actual functionality :)

@Nirusu Nirusu requested a review from thomasten June 21, 2022 08:37
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
ego/cli/bundle.go Outdated Show resolved Hide resolved
ego/cli/cli.go Outdated Show resolved Hide resolved
ego/cli/cli.go Outdated Show resolved Hide resolved
ego/cli/bundle.go Outdated Show resolved Hide resolved
ego/cli/bundle.go Outdated Show resolved Hide resolved
ego/cli/bundle.go Outdated Show resolved Hide resolved
ego/cli/bundle.go Outdated Show resolved Hide resolved
ego/bundle/main.go Outdated Show resolved Hide resolved
@Nirusu Nirusu force-pushed the feat/bundle branch 2 times, most recently from 8e14578 to a2ee921 Compare June 27, 2022 10:15
@Nirusu
Copy link
Contributor Author

Nirusu commented Jun 27, 2022

Ready for re-review. I merged the common code from cli and bundle into internal/launch. I also put the untar logic in there, given that it is kinda launch related and I did not want to create yet another utils package just for one function.

Also, for the output filename I did go the hybrid way: If no output name is specified, take the original name and append "-bundle", otherwise use the specified output name.

I also separated the "utils.go" I created before into separate files that are more distinctive. That should look better and make more sense I believe.

@Nirusu
Copy link
Contributor Author

Nirusu commented Jun 27, 2022

Sorry for all the additional commits. I now also completely moved the Marblerun launch to the internal package, so that cli/run.go really mainly is a caller around the new launch package. That's mainly just for consistency, so that all launch commands are at one place, even if the CLI is so far the only caller for the Marblerun launch mode.

CMakeLists.txt Outdated Show resolved Hide resolved
ego/cmd/bundle/main.go Outdated Show resolved Hide resolved
ego/internal/launch/launch.go Show resolved Hide resolved
ego/cmd/ego/main.go Outdated Show resolved Hide resolved
ego/cmd/ego/main.go Show resolved Hide resolved
ego/internal/launch/errors.go Outdated Show resolved Hide resolved
ego/internal/launch/launch.go Outdated Show resolved Hide resolved
ego/cmd/bundle/main.go Outdated Show resolved Hide resolved
ego/cli/bundle.go Outdated Show resolved Hide resolved
ego/cli/bundle.go Outdated Show resolved Hide resolved
@Nirusu Nirusu requested a review from thomasten June 28, 2022 12:53
Copy link
Member

@thomasten thomasten left a comment

Choose a reason for hiding this comment

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

Nice work, thanks!

@thomasten thomasten merged commit 0ca5bf3 into master Jun 29, 2022
@thomasten thomasten deleted the feat/bundle branch June 29, 2022 09:04
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