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

feat(ci): publish platform specific binaries #850

Merged
merged 9 commits into from
Dec 14, 2021
Merged

Conversation

numToStr
Copy link
Contributor

@numToStr numToStr commented Dec 7, 2021

Summary of changes:


Some questions I have

  • Platform suffix in the build path (ie. bin/{Linux,Windows,macOS}) bothers me a little as this is also reflected inside the release assets. Also, IMO these are kinda redundant now. So, Are you ok with removing the suffix? So that assets could just have binary w/o any directories.
-- Current
"bin/Linux" -- Bin dir
"lua-language-server-linux/linux/lua-language-server" -- inside asset
--     ^                     ^
-- asset name         platform suffix    

-- Wanted
"bin" -- bin dir
"lua-language-server-linux/lua-language-server" -- asset
-- or
"lua-language-server-linux/bin/lua-language-server" -- alternative structure

Closes #849

@numToStr
Copy link
Contributor Author

numToStr commented Dec 7, 2021

I think I might also need to update the wiki :)

@clason
Copy link

clason commented Dec 7, 2021

Note that the binaries are not enough to have a functional installation; you also need the runtime scripts: #849 (comment)

@clason
Copy link

clason commented Dec 7, 2021

And +1 on flattening the hierarchy for the binary; that would make packaging for Homebrew easier!

@numToStr
Copy link
Contributor Author

numToStr commented Dec 7, 2021

  1. copy (recursively) main.lua, debugger.lua, locale, meta, and script to build

@clason Are you talking about this?

@clason
Copy link

clason commented Dec 7, 2021

yes

@numToStr
Copy link
Contributor Author

numToStr commented Dec 7, 2021

Ok. I'll give it a shot.

@clason
Copy link

clason commented Dec 7, 2021

Also, a standard archive with the full (including submodules) sources for each release would be nice for Homebrew ;)

@numToStr
Copy link
Contributor Author

numToStr commented Dec 7, 2021

Isn't the source zip automatically uploaded in the release?

@clason
Copy link

clason commented Dec 7, 2021

I don't think that includes the submodules? At least it doesn't for the tags.

(Not at all a dealbreaker, but if that's an easy add to your action, it'd be nice.)

@numToStr
Copy link
Contributor Author

numToStr commented Dec 7, 2021

I don't think that includes the submodules? At least it doesn't for the tags.

Just checked. It doesn't even have the .git folder XD

(Not at all a dealbreaker, but if that's an easy add to your action, it'd be nice.)

I suppose I could do that :)

@clason
Copy link

clason commented Dec 7, 2021

I don't want the .git folder, just the pulled dependencies ;)

@numToStr
Copy link
Contributor Author

numToStr commented Dec 7, 2021

I don't want the .git folder, just the pulled dependencies ;)

Got it. I'll probably tackle this tomorrow :p

@actboy168
Copy link
Collaborator

Platform suffix in the build path (ie. bin/{Linux,Windows,macOS}) bothers me a little as this is also reflected inside the release assets. Also, IMO these are kinda redundant now. So, Are you ok with removing the suffix? So that assets could just have binary w/o any directories.

I guess the bin directory contains platform suffix, because the release version of VSCode needs to contain all platform binaries.

But VSCode already supports platform specific extension. If lua-language-server can support this feature, it will no longer be necessary to include multiple platform binaries in one release.

@numToStr
Copy link
Contributor Author

numToStr commented Dec 7, 2021

If lua-language-server can support this feature, it will no longer be necessary to include multiple platform binaries in one release.

Yes, the goal is to release platform specific binary packages so that anyone can play with lua-server as quickly as possible.

But VSCode already supports platform specific extension.

I don't know anything about vscode extensions but if it supports platform specific extensions then we are in good position.

@numToStr numToStr marked this pull request as draft December 7, 2021 20:46
@sumneko
Copy link
Collaborator

sumneko commented Dec 8, 2021

I will try to modify the VSCode release process and remove the <platform> folder

@numToStr numToStr force-pushed the feat/ci branch 4 times, most recently from c95db9f to f89c86a Compare December 8, 2021 06:15
@numToStr
Copy link
Contributor Author

numToStr commented Dec 8, 2021

@sumneko Should I remove the <platform> folder here? I suppose this is the line https://github.com/sumneko/lua-language-server/blob/247377652b61fd21f0a16ff8fd15bc8c54e3e080/make.lua#L5

@sumneko
Copy link
Collaborator

sumneko commented Dec 8, 2021

@sumneko Should I remove the <platform> folder here? I suppose this is the line

https://github.com/sumneko/lua-language-server/blob/247377652b61fd21f0a16ff8fd15bc8c54e3e080/make.lua#L5

There are still some problems with modifying the directory structure, we need to keep it unchanged first.

@numToStr
Copy link
Contributor Author

numToStr commented Dec 8, 2021

@clason I've added the runtime files inside the release assets and also added the tagged version in the assets name. See https://github.com/numToStr/lua-language-server/releases/tag/0.0.0-pre.9

@numToStr
Copy link
Contributor Author

numToStr commented Dec 8, 2021

There are still some problems with modifying the directory structure, we need to keep it unchanged first.

@sumneko Understood. I'll let you update the directory structure :)

@numToStr numToStr force-pushed the feat/ci branch 2 times, most recently from 7953ce0 to 7b47842 Compare December 8, 2021 14:50
@numToStr
Copy link
Contributor Author

numToStr commented Dec 8, 2021

Now the submodules asset is also ready :) https://github.com/numToStr/lua-language-server/releases/tag/0.0.0-pre.12

@numToStr numToStr marked this pull request as ready for review December 8, 2021 15:16
@sumneko
Copy link
Collaborator

sumneko commented Dec 14, 2021

Which version do you want to revert to? ./bin/Linux/lua-language-server or ./Linux/lua-language-server or other?

@numToStr
Copy link
Contributor Author

Which version do you want to revert to? ./bin/Linux/lua-language-server or ./Linux/lua-language-server or other?

To this ./bin/Linux/lua-language-server as It was before. Thanks :)

@sumneko
Copy link
Collaborator

sumneko commented Dec 14, 2021

It should be reverted.

@numToStr
Copy link
Contributor Author

Thanks, @sumneko. I'll quickly rebase my PR.

@numToStr
Copy link
Contributor Author

@sumneko I think I am finished with this PR. You can check the latest run and release assets

@sumneko
Copy link
Collaborator

sumneko commented Dec 14, 2021

What is submodules.zip ?

@numToStr
Copy link
Contributor Author

What is submodules.zip?

This contains all the submodules sources for homebrew.

@sumneko
Copy link
Collaborator

sumneko commented Dec 14, 2021

I think the ./build folder should be excluded

@numToStr
Copy link
Contributor Author

Done.

@sumneko
Copy link
Collaborator

sumneko commented Dec 14, 2021

./log should also be excluded.
I still not understand why it is needed, the ./3rd/lovr-api is so large

@numToStr
Copy link
Contributor Author

./log should also be excluded. I still not understand why it is needed, the ./3rd/lovr-api is so large

@clason asked me for the submodule asset for homebrew distribution. I could remove it if you want.

@sumneko
Copy link
Collaborator

sumneko commented Dec 14, 2021

./log should also be excluded. I still not understand why it is needed, the ./3rd/lovr-api is so large

@clason asked me for the submodule asset for homebrew distribution. I could remove it if you want.

I used to think of homebrew as darwin, although they didn't look same at all.
If it is necessary for package management, there is no problem (I am not paying for the file size and bandwidth anyway)

@sumneko
Copy link
Collaborator

sumneko commented Dec 14, 2021

It seems good, thank you!

@sumneko sumneko merged commit ee1e3ab into LuaLS:master Dec 14, 2021
@numToStr numToStr deleted the feat/ci branch December 14, 2021 15:20
@sumneko sumneko mentioned this pull request Dec 14, 2021
5 tasks
@clason
Copy link

clason commented Dec 14, 2021

./log should also be excluded. I still not understand why it is needed, the ./3rd/lovr-api is so large

@clason asked me for the submodule asset for homebrew distribution. I could remove it if you want.

I used to think of homebrew as darwin, although they didn't look same at all. If it is necessary for package management, there is no problem (I am not paying for the file size and bandwidth anyway)

I just need the 3rd/luamake and 3rd/bee subdirectories for building. The idea is to be able to just download the release and call luamake rebuild without needing a git submodule init update.

I used to think of homebrew as darwin, although they didn't look same at all.
If it is necessary for package management, there is no problem (I am not paying for the file size and bandwidth anyway)

Homebrew used to be macOS only, but they have joined with the linuxbrew project, so now they build everything for macOS and Linux from the same formula (hence needing to distinguish the platforms there).

@clason
Copy link

clason commented Dec 14, 2021

To make clear, it would simplify the homebrew formula if the v2.6.0.tar.gz release tarball included the submodules needed for building it, but it is not necessary (homebrew can clone a repo and update submodules as part of the build pipeline, like it does for the current formula I submitted). So if that is at all awkward to do here, just don't worry about including submodules.

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.

[Feature Request] All In One Releases
4 participants