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

Add packaging using new implementation #20

Merged
merged 15 commits into from
Sep 9, 2019
Merged

Add packaging using new implementation #20

merged 15 commits into from
Sep 9, 2019

Conversation

provokateurin
Copy link
Member

Using the new implementation from here
Previously was #15

@pchampio
Copy link
Member

I've quickly looked at the implementation and the code looks very good to me.
I'll test it out later this week.

cmd/packaging.go Show resolved Hide resolved
cmd/packaging.go Show resolved Hide resolved
cmd/packaging.go Outdated Show resolved Hide resolved
cmd/packaging.go Outdated Show resolved Hide resolved
cmd/packaging.go Outdated Show resolved Hide resolved
cmd/packaging.go Outdated Show resolved Hide resolved
cmd/common.go Outdated Show resolved Hide resolved
cmd/packaging.go Outdated Show resolved Hide resolved
cmd/packaging.go Show resolved Hide resolved
cmd/packaging.go Outdated Show resolved Hide resolved
cmd/packaging.go Outdated Show resolved Hide resolved
@GeertJohan
Copy link
Member

Nice work! I've left some comments.
I would also like to have --version-name and --version-number overrides for hover build *, but perhaps that's something that can be done later, not a must-have for this PR.

@pchampio
Copy link
Member

pchampio commented Sep 7, 2019

@GeertJohan can we get one last review before this gets merged?


edit: misclick the closed&comment button 🥇

@pchampio pchampio closed this Sep 7, 2019
@pchampio pchampio reopened this Sep 7, 2019
@GeertJohan
Copy link
Member

GeertJohan commented Sep 8, 2019

The current implementation writes a lot of files that are used to build the deb into go/packaging/linux-deb/usr. I think this should be moved to a tmp directory.

edit: Its in the .gitignore, I missed that.

@provokateurin
Copy link
Member Author

Yes, but don't really like it

@GeertJohan
Copy link
Member

GeertJohan commented Sep 8, 2019

Awesome job @jld3103, I just packaged one of my applications as a deb, very cool.

I feel there are a few last points that need to be addressed before we can release this into the wild.

  1. For linux-deb on a project called "dashboard", all files are put in /usr/bin/dashboardfiles/.... It's not correct to put directories in /usr/bin. I think it would be cleaner to put the contents of dashboarfiles in /usr/lib/dashboard/... https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s06.html

  2. Directories inside go/packaging are now used as working directory for building the output. I think that in this case we should use a tmpdir and copy files from our go/packaging/<type> structure into the tmp dir, then use that in further processing. This way, we don't have to add complex lines to .gitignore, and we can allow users to add custom content to go/packaging/* folders. (Related user request) Also, we don't have to clean up. Lets say that I rename "dashboard" to "myApp", then what happens to go/packaging/linux-deb/usr/bin/dashboardfiles? And will myAppfiles be added to the gitignore? We avoid all these issues by using a tmp build dir.

A nice-to have, but not required to merge this PR:
The version field in DEBIAN/control isn't overwritten on build linux-deb. I think it should be, so that users can run hover build linux-deb --version-number 1.2.3 (when that flag is introduced). If that flag is not used, then the version number from pubspec.yaml should be used.

@provokateurin
Copy link
Member Author

@GeertJohan Done.

@GeertJohan GeertJohan merged commit d0a8cce into go-flutter-desktop:master Sep 9, 2019
@GeertJohan
Copy link
Member

Thanks a lot @jld3103 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants