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

implement cobra cli for apps & integrate apps with merged binary compilation #1704

Merged
merged 22 commits into from
Feb 11, 2024
Merged

implement cobra cli for apps & integrate apps with merged binary compilation #1704

merged 22 commits into from
Feb 11, 2024

Conversation

0pcom
Copy link
Collaborator

@0pcom 0pcom commented Dec 19, 2023

This PR intends to demonstrate the inclusion of apps in a merged compilation of a single skywire binary.

Cobra cli has been implemented for the skywire apps, and they can be started successfully by the visor.

The remaining task is to include the binaries in a merged compilation and alter the config of the visor to support starting the apps via subcommands of the merged compilation.

image

@0pcom
Copy link
Collaborator Author

0pcom commented Dec 19, 2023

image

@0pcom
Copy link
Collaborator Author

0pcom commented Dec 20, 2023

Note at this point, with the apps merged into a single binary, the size of the single binary is ~35MiB
Compared to a total size of >100MiB for all binaries when separately compiled.

So the size of the installation is reduced to approx. 1/3rd of its original size with separate binaries

@0pcom
Copy link
Collaborator Author

0pcom commented Dec 20, 2023

instead of changing how the apps are launched to accommodate the apps being part of the same binary, it is possible to simply use a similar method as the one used by make run-source

i.e. with scripts in place of the app binaries that would call the subcommand for that app from the main skywire binary.

see the scripts here for an example of what i'm referring to:
https://github.com/skycoin/skywire/tree/develop/scripts/_apps

this may be the optimal approach for now.

@0pcom
Copy link
Collaborator Author

0pcom commented Dec 20, 2023

@mrpalide can you confirm that the normal separate compilation of the app binaries is still working since I've implemented cobra for them? They are working for me on linux. Let's merge this after you confirm it's working as expected.

@mrpalide
Copy link
Contributor

All apps not work on Mac and Windows due these same errors:

[2023-12-21T16:01:25.734304+03:30] ERROR (STDERR) [proc:vpn-client:6666d9e49ea94cfc829957e7ba76e461]: 2023/12/21 16:01:25 Failed to execute command: unknown shorthand flag: 's' in -srv
[2023-12-21T16:01:26.003133+03:30] ERROR (STDERR) [proc:skychat:68f54d016c0a411290c042cac73767a1]: 2023/12/21 16:01:26 Failed to execute command: unknown shorthand flag: 'a' in -addr
[2023-12-21T16:01:26.459688+03:30] ERROR (STDERR) [proc:skysocks-client:2792644592ff49b0ab70f0a1b58519fb]: 2023/12/21 16:01:26 Failed to execute command: unknown shorthand flag: 'a' in -addr
[2023-12-21T16:05:36.878205+03:30] ERROR (STDERR) [proc:skysocks:7434b5801cba4683ac124a8b80000bca]: 2023/12/21 16:05:36 Failed to execute command: unknown shorthand flag: 'p' in -passcode
[2023-12-21T16:05:50.882658+03:30] ERROR (STDERR) [proc:vpn-server:c9ad42d354854ce68530f6bae5bb7de7]: 2023/12/21 16:05:50 Failed to execute command: unknown shorthand flag: 'p' in -passcode

Flags not passed correctly.

btw, I'm not agree with this merging. It make everything complex between apps and visor. Also it make issue on unofficial apps that users and 3rd parties will provide later.

@0pcom
Copy link
Collaborator Author

0pcom commented Dec 21, 2023

btw, I'm not agree with this merging. It make everything complex between apps and visor. Also it make issue on unofficial apps that users and 3rd parties will provide later.

I think using cobra for the cli will allow for the apps to be more easily tested directly, outside the context of the visor. We can add later a flag to mock the visor or enable direct connections for local testing.

It's not a requirement that the apps must be compiled together into a single binary, I just want the option to do so. Adding cobra to the apps should not be a breaking change, regardless.

I understand what you mean about what apps are included by default. it's not necessary that we change the releases to use the merged binary.

I will attempt to fix these errors and speak with you more to address the concerns you expressed.

I think there must be an easier way to determine at compile time which apps we will include or not. it may even be possible to simply hide or disable apps selectively which are compiled into the same binary, ideally we could choose what apps to include in the merged binary without hard-coding them. but i will look into this. Thanks for your feedback.

@0pcom
Copy link
Collaborator Author

0pcom commented Dec 21, 2023

with make run-source the apps are working for me on linux
image

which is very strange, as I think I know why you were getting an error, but I don't know why I'm not getting the same error on linux.

I simply need to adjust cobra to accept flags in the same format as the "flags" package uses.
...

apparently it is not possible to do this.
spf13/cobra#740

@mrpalide
Copy link
Contributor

I still disagree with this PR.

Separating the app (by adding Cobra) from the visor is contrary to the whole idea of skywire, that the applications run under the supervision of a visor and based on the connection with it. In general, the philosophy of the apps here is to inherit the confidentiality, security and encryption of Skywire and to be genetically associated with this property! But isolation and separate implementation completely destroys this vision.

If the VPN client and server need to work separately from Skywire, and only need a private and public key pair, that's a whole other project.

I feel that this idea needs to be revised more.

@0pcom
Copy link
Collaborator Author

0pcom commented Dec 28, 2023

I still disagree with this PR.

Separating the app (by adding Cobra) from the visor is contrary to the whole idea of skywire, that the applications run under the supervision of a visor and based on the connection with it. In general, the philosophy of the apps here is to inherit the confidentiality, security and encryption of Skywire and to be genetically associated with this property! But isolation and separate implementation completely destroys this vision.

If the VPN client and server need to work separately from Skywire, and only need a private and public key pair, that's a whole other project.

I feel that this idea needs to be revised more.

The point is not for them to be used separately, or independently from the visor.

Using cobra:

  • can make it easier to test or develop individual apps - i.e. if they can be invoked with a flag that mocks the visor / deployment, so that they can be tested locally.

Currently, apps can only be tested when the deployment works, and the deployment not working has made it difficult for Archimedes to work on the skychat app in the past.

This PR does not add a flag to mock the visor or deployment, but it would be easy to do that subsequently.

  • We already use cobra for every other cli help menu - so this standardizes the apps to be the same as all the existing binaries / commands

Simply implementing cobra for the apps does not force them to be compiled with the other binaries, but it gives me the option for a merged compilation.

The apps can still work the same way as they do currently, the point is to make them able to work as a merged compilation by adding them to the command structure (importing the app commands package) and saving the space of independently compiled extra binaries.

So this PR hopes to retain the existing functionality while adding compatibility for an alternative compilation, which will remain optional.

In the second place, the way that the app config is handled by the visor is very complicated. No one is going to write a skywire application without changes to the source code which will support their application's flags. And though it is still possible to write an example app and manually add it's config, it's still much more work to do something like that versus skyfwding or potentially just using a dmsg client directly in an application and allowing for a proxy config that will use the skywire proxy.

Synth wants a simple way to configure most or many applications to use skywire. And we more or less have that with skyfwd.

I think you misunderstand this PR, it's goals and intentions.

I want to add cobra cli to the apps and have them work the exact same way as they currently work on develop.

Adding cobra will support an optional merged compilation of the binaries. With emphasis on 'optional' and 'retains current functionality'

@0pcom 0pcom merged commit 247f30b into skycoin:develop Feb 11, 2024
3 checks passed
@0pcom 0pcom deleted the merged-apps branch February 11, 2024 20:26
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