-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: packaging #4270
base: master
Are you sure you want to change the base?
feat: packaging #4270
Conversation
Packaging test
Packaging test
fix: appimage.sh
feat: add notes.md
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.
why not invoke the cmake script from that repo instead? basically clone it first and then run whatever build steps you want.
You can even create a custom github job from that repo to get a cleaner interface.
pros:
- avoid adding a lot of files that are only relevant for distribution
- won't clutter the repo's history with
ci
commits - adjusting build steps faster and easier
cmake needs to be together with the source code for homebrew |
Cloning would make the build artifacts non-deterministic and Homebrew compares against a shasum. See Homebrew/homebrew-core#132446 for more context. Specifically this comment. |
@osalbahr, thanks for adding more context! I'm still not sure about adding all those files into the repo, I was hoping we can get away with doing something along the these lines class Lunarvim < Formula
desc "IDE layer for Neovim. Completely free and community driven"
homepage "https://www.lunarvim.org"
url "https://github.com/LunarVim/LunarVim/archive/refs/tags/1.3.0.tar.gz"
version "1.3.0"
sha256 "2b6cefdf76d42e2d349381e63d42fa3c3e2527da25a464cb0a2af47bdea9e2a0"
license "GPL-3.0-or-later"
head "https://github.com/LunarVim/LunarVim.git", branch: "master"
resource "LunarVim-mono" do
url "https://github.com/LunarVim/LunarVim-mono/archive/refs/tags/stable.tar.gz" # replace stable with 1.3.0
sha256 "4b63e968655d9a71ff32fdca653a56bd560555c1780d704669df578048b5f808"
end
depends_on "cmake" => :build
depends_on "fd"
depends_on "neovim"
depends_on "node"
depends_on "python@3.11"
depends_on "ripgrep"
depends_on "tree-sitter"
def install
resources.each do |r|
r.stage(buildpath/"packaging"/r.name)
end
cd "packaging" do
# ..
end
end
test do
assert shell_output("#{bin}/lvim -v").start_with?("NVIM")
assert_equal "lmao", pipe_output("#{bin}/lvim -Es +%print", "lmao\n").strip
end
end @ChristianChiarulli, what's your stance on this? |
I could probably make this pr quite a bit smaller. the build step would stay here, that would leave 151 loc (7 files 2 of which are icons) in this pr. the rest (github workflow, appimage, Packaging.cmake, BundlePlugins.cmake) would go to the LunarVim-mono repo. |
Why not keep all "lunarvim-packaging" scripts/tools in a separate repo? You'd clone it into Keep in mind we probably need more scripts for integrating with other package formats (e.g. nix, scoop, winget,. etc.) so this packaging repo will only get more complex. |
I can agree with all the packaging stuff begin in a separate repo. just the build cmake would stay in the core, it makes packaging for others way easier and it makes sense for it to be versioned. I think it would be a nice compromise |
Not sure which cmake files you mean here, since the main Although, looking more into it, I see some ways to minimize the added files. Please try moving the cmake logic into just two files: |
so now these files would be in the core:
the rest would go to the lunarvim-mono repo |
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.
- Let's merge the icon files into
utils/desktop
CMakeLists.txt
should not be on the top level, it makes it look like it's necessary to use it to "build" the repo/config (I think should be possible to do)
I tried to do that, and it seems like a pain because you'd need to access paths outside of the cmake project, but i'll do a bit more research on it |
I think this is a great feature that would benefit Homebrew and non-Homebrew users alike!
Oh, makes sense. I actually don’t use the AUR very often so I don’t know how it works. |
@LostNeophyte, I actually figured out a pretty simple solution that will, hopefully, satisfy everyone!
This will avoid any confusion about having a |
Sounds good to me, do I still put the ci workflow, appimage script and CPack in the lvim-mono repo? |
what is used for building LunarVim from source? |
we don't build it, lunarvim doesn't require building. the installer only moves files to correct places. well, maybe you could argue that the |
That’s great. If you list the exact step-by-step commands, we can just put these commands in the Homebrew formula. It seems no changes to the repo would be needed. Here is an simple example of a “def install” they can be just shell commands: https://docs.brew.sh/Formula-Cookbook#grab-the-url but it would be easier to maintain if the commands were wrapped in a “./configure --PREFIX=” and then “make install” although not required. The added benefit would be that someone manually installing from the master branch can also follow the same steps in the Homebrew formula. You can think of the “def install” section as a general and reproducible way of manually installing from source. I think maybe that the script in the LunarVim basically does that? If so then we can probably use it. |
yup, I don't want the commands to be in the formula, as they'd need to be replicated in many places.
it's close, not quite there. I think it's complex enough and I'd rather not add a new feature to it |
Great point. I agree.
Interesting. I haven’t done much development on windows this doesn’t surprise me.
That’s a nice workaround! Not sure if doing |
no, that's fine since it goes into however, we should look into making it a github action if the scripts end up being too long/complex.
it's something like this diff --git a/Makefile b/Makefile
index 1f6844c5..bc842db5 100644
--- a/Makefile
+++ b/Makefile
@@ -39,4 +39,9 @@ style-sh:
test:
bash ./utils/ci/run_test.sh "$(TEST)"
-.PHONY: install install-neovim-binary uninstall lint style test
+packaging-brew:
+ cp utils/packaging/CMakeLists.txt.in CMakeLists.txt
+ cmake --configure . -B build -D BUNDLE_PLUGINS=1
+ cpack
+
+.PHONY: packaging-brew install install-neovim-binary uninstall lint style test |
looks like homebrew PR is stale, might need another one to bring life to it. |
@kylo252 we'll need to remember to change version strings before releases, i might create scripts/gh action to make it easier LunarVim/utils/packaging/CMakeLists.txt.in Lines 11 to 14 in 0465cf9
|
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.
almost there! 🔥
set(XDG_ROOT ${CMAKE_BINARY_DIR}/xdg_root) | ||
set(LVIM_XDG_DATA_HOME ${XDG_ROOT}/share) | ||
set(LVIM_XDG_CONFIG_HOME ${XDG_ROOT}/.config) | ||
set(LVIM_XDG_CACHE_HOME ${XDG_ROOT}/.cache) |
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.
I don't have the full picture but I feel like this would require moving the plugins again since xdg_root
is hardcoded in the path.
This should be easy to verify though, so please show the intended folder structure.
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.
these folders are only used for the neovim instance to run. then the plugins are taken from there:
LunarVim/utils/packaging/cmake/BundlePlugins.cmake
Lines 52 to 54 in f1e610d
install(DIRECTORY | |
"${LUNARVIM_RUNTIME_DIR}/site/pack/lazy/opt/" | |
DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/lunarvim/plugins/) |
I didn't test it on windows yet, when i do I'll check if the |
tested on windows and linux |
utils/packaging/CMakeLists.txt.in
Outdated
if(PACKAGE_FOR_WINDOWS OR WIN32) | ||
set(LVIM_INPUT_BIN_NAME lvim.ps1) | ||
set(LVIM_BIN_NAME lvim.ps1) | ||
set(BASE_DIR_VAR "\$(Resolve-Path \"\$PSScriptRoot\\..\\${CMAKE_INSTALL_DATAROOTDIR}\\lunarvim\")") | ||
else() | ||
set(LVIM_INPUT_BIN_NAME lvim.template) | ||
set(LVIM_BIN_NAME lvim) | ||
set(BASE_DIR_VAR "\$(readlink -f \"\$(dirname \"\$(realpath \"\$0\")\")/..\")/${CMAKE_INSTALL_DATAROOTDIR}/lunarvim") |
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.
how about using PROJECT_SOURCE_DIR
? unless you're referencing the destination, then it should just be using CMAKE_INSTALL_xxx
.
if it's easier, you could make it "build" the scripts and then you can install them according to installation rules.
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.
it's for the destination, it needs to be relative for the appimage, the paths are random (e.g /tmp/.mount_lvim(1mk5W7d/usr/share/lunarvim)
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.
it needs to be relative for the appimage
do you mean inside the appimage rather than relative? What I'm trying to say is that having /../
somewhere in path doesn't necessarily make it relative. I don't see how BASE_DIR_VAR
wouldn't look like this /some/path/../share/lunarvim
, which might even get resolved when using readlink
.
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.
we can't use a fixed absolute path https://docs.appimage.org/reference/best-practices.html?highlight=relocatable#binaries-must-not-use-compiled-in-absolute-paths
the suggested way to deal with this is to make paths relative to the executable, so here the path is PATH_TO_EXE/../${CMAKE_INSTALL_DATAROOTDIR}/lunarvim
(/tmp/.mount_lvim(1mk5W7d/usr/bin/../share/lunarvim
in appimage, /usr/bin/../share/lunarvim
in a normal installation)
@@ -184,7 +184,8 @@ function setup_shim() { | |||
New-Item "$INSTALL_PREFIX\bin" -ItemType Directory | Out-Null | |||
} | |||
|
|||
Copy-Item -Force "$env:LUNARVIM_BASE_DIR\utils\bin\lvim.ps1" "$INSTALL_PREFIX\bin\lvim.ps1" | |||
((Get-Content -path "$env:LUNARVIM_BASE_DIR\utils\bin\lvim.ps1" -Raw) -replace '@BASE_DIR_VAR@','$env:LUNARVIM_RUNTIME_DIR\lvim') | |
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.
was this difficult to handle with cmake as well?
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.
I didn't think this one through, I also made this relative but appimage isn't for windows 🤦
adds the cmake setup from LunarVim-mono.
the ci job will create nightly releases on a schedule and create stable releases on tag push, if the stable release already exists it will upload files to it.
you can see it in action here
https://github.com/LostNeophyte/LunarVim/releases
https://github.com/LostNeophyte/LunarVim/actions/runs/5435290043