Skip to content

Multi and wildcard target support (v2) #67

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

Conversation

provokateurin
Copy link
Member

This adds support for multi and wildcard target support.
This uses the previous way for single targets, but makes multi and wildcard targets available using --packaging-targets for hover init-packaging and --build-targets for hover build.

@pchampio You said that nobody really needs this feature, but I actually need it. I'm building my app using CI where this is used to build all app types.
Anyway this is only an addition to the old format, because as you said the previous way for single targets was much better.
Also adding this target system makes it easier when flutter gets the ability to build 32-bit bundles for Raspberry Pi etc. (Which I hope will come in the near future). Then we only need to add a new field to a Target.

@pchampio
Copy link
Member

pchampio commented Nov 20, 2019

@jld3103
I haven't looked at the code, but here is my reasoning:

Introduction: The Leverage mental model (taken from Justin M. Keyes)

Leverage = (impact / cost):

  • impact = total effect (usage x time)
    helping the most number of people for the longest amount of time.
  • cost = effort, human-hours, maintenance burden, ...
    costs that are involved in making a change.

Low leverage: shallow features

  • if it helps a few people but it's hard to implement then his low leverage.

High leverage: deep extensibility

  • something that helps a few people but it's like trivial to implement then it's high leverage.

Analysis of the project

The Hover source code is spitted across 36 files [1].
Hover has a total of 3765 lines of Golang code [2]. In comparison go-flutter has 4142 loc, and it's the core project, find the issue^^.

Analysis of the PR

  • In term of usage, you are currently the only one requesting for this hover feature, I guess other peoples are using bash script.

  • In term of time, hover run must be as simple and as complete as possible because it's the most used command. Also, in the future, for production usage 'flutter desktop' will eat a high majority of go-flutter users. In the current state, hover is great because it allows for quick desktop development/prototyping. With the fact that flutter for desktop in general (go-flutter, 'flutter desktop', flutter-rs) isn't ready for production use, I don't think this will be used for an extended period of time. Plus users might prefer using bash scripting (because who reads docs).

  • In term of cost, this PR touches 27 of the current 36 Golang files. Adding +975 loc and removing -537 loc. This PR modifies roughly ~50% of the hover project. This PR (notably because of the -537 loc refactor) is very costly to review, and adds 439 loc to maintain (~16% of hover).

Conclusion

Based on the analysis of this PR, when we apply the above leverage mental model this PR has a low leverage score. In comparison, #59 was much easier to implement (low cost) and solved issues raised by 2 different actors go-flutter-desktop/go-flutter#296 and go-flutter-desktop/go-flutter#203 (higer impact) (Note that those actors aren't Collaborator, and thus biased in a different manner)

On your previous PR #52, I suggested 2 propositions:

  • First, such wildcard support shouldn't be possible in hover init-packaging. The reason is that hover init-packaging only creates template files that needs to later be added to git, thus users only run hover init-packaging for each packaging format once, no wildcard support is needed because it's not a tedious, time-consuming task.
  • Second, wildcard support can be implemented using a very simple hover build all command. all defines all hover init-packaging-ed package (excluding linux, windows, darwin). So for example, if the user has initialized
    windows-msi, darwin-bundle, and linux-appimage, hover build all will build be equivalent to:
$ hover build all
$ # vs
$ hover build windows-msi && hover build darwin-bundle && hover build linux-appimage
$ #  (excluding `linux-deb`, `linux-snap`,.. because they where not initialized)

What do you think of this proposal?

-------------------------------------------------------------------------------
 Language            Files        Lines         Code     Comments       Blanks
-------------------------------------------------------------------------------
 Dart                    1           10            8            0            2
 Go                     36         4529         3765          178          586
 JSON                    1           18           18            0            0
 Markdown                1          127          127            0            0
 YAML                    1            5            3            2            0
-------------------------------------------------------------------------------
 Total                  40         4689         3921          180          588
-------------------------------------------------------------------------------

@provokateurin
Copy link
Member Author

I understand. I guess you're right.

@pchampio
Copy link
Member

How about the hover build all proposition?

@provokateurin
Copy link
Member Author

That should be the way to go.

@provokateurin provokateurin deleted the feature/target-wildcards branch December 25, 2019 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants