-
Notifications
You must be signed in to change notification settings - Fork 51
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
Conversation
8e14578
to
a2ee921
Compare
Ready for re-review. I merged the common code from 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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, thanks!
Proposed changes
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 :)