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: add builtin build step #18

Merged
merged 35 commits into from
Jun 29, 2024
Merged

feat: add builtin build step #18

merged 35 commits into from
Jun 29, 2024

Conversation

vhyrro
Copy link
Contributor

@vhyrro vhyrro commented Jun 1, 2024

This PR will add a functional builtin build step for rocks.

Additionally, this PR refactors a few bits and bobs related to the build process to make everything cleaner and more readable with less duplication :)

@vhyrro
Copy link
Contributor Author

vhyrro commented Jun 2, 2024

I decided to do something slightly controversial but I believe it's a good decision in the long run (cc @mrcjkb) - I decided to break the rocktree schema and implemented my own.

Instead of the all-over-the-place directory structures I've now reduced a rock tree to the following:

/home/vhyrro/.local/share/rocks/
└── 5.1
    └── neorg@scm-1
        ├── etc // contains extra files (documentation)
        ├── lib // contains shared library objects
        └── src // contains source code for the rock

All rocks have an associated version number and the directories that distinguish various parts of the rock are very clear-cut and easy to understand.

Below are some extra bits of information about this change:

  • A rocks server and a rocks tree is no longer "the same thing" - trying to merge them together like luarocks does it makes the whole implementation much more complicated. A rocks server should host rockspecs or packed rocks, whereas a rocks tree should only contain unpacked rocks. I do not plan on breaking the directory layout of servers, as those are important for interoperability with luarocks. Rather, I'm only interested in changing the directory structure of rocks trees.
  • Rocks stores its data in a different place to luarocks, so keeping rocks tree compatibility is not really incentivized.
  • If a user wants to mirror their old rocks tree in rocks then they can simply reinstall all their favourite rocks using our CLI. The end result is the same, the backend is different.

@vhyrro
Copy link
Contributor Author

vhyrro commented Jun 3, 2024

All the boilerplate is in place now... all that's left is fixing a weird deserialization issue that I can't figure out quite just yet.

@vhyrro
Copy link
Contributor Author

vhyrro commented Jun 3, 2024

Leaving it as-is for now, I'm finding it hard to debug the issue :p

@vhyrro vhyrro marked this pull request as ready for review June 4, 2024 13:07
@vhyrro
Copy link
Contributor Author

vhyrro commented Jun 4, 2024

This is ready to review now. Not all features are fully set in stone yet, for instance I need to figure out how to read source.defines and source.incdirs. Additionally, more url = "schema://"s need to be supported (only git is supported for now). I'll be working on those in the meantime, exciting to see this feature in a working state :)

config
.lua_version
.as_ref()
.unwrap_or(&crate::config::LuaVersion::Lua51),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lua_version should be read from a configuration file instead of defaulting to Lua 5.1 here :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might also be nice to be able to set it from an environmen variable

Copy link
Member

@mrcjkb mrcjkb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[review incomplete] To be continued at a later time 😃

lua-cjson.rockspec Outdated Show resolved Hide resolved
rocks-bin/src/build.rs Outdated Show resolved Hide resolved
rocks-bin/src/main.rs Show resolved Hide resolved
rocks-bin/src/main.rs Outdated Show resolved Hide resolved
rocks-lib/src/build/mod.rs Outdated Show resolved Hide resolved
config
.lua_version
.as_ref()
.unwrap_or(&crate::config::LuaVersion::Lua51),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might also be nice to be able to set it from an environmen variable

rocks-lib/src/rockspec/build/builtin.rs Outdated Show resolved Hide resolved
rocks-lib/src/tree/mod.rs Show resolved Hide resolved
rocks-lib/src/rockspec/build/builtin.rs Outdated Show resolved Hide resolved
@vhyrro vhyrro changed the title Add builtin build step feat: add builtin build step Jun 9, 2024
@vhyrro
Copy link
Contributor Author

vhyrro commented Jun 9, 2024

I recognize that some parts of the code are slightly smelly and imperfect (including the large amount of for loops hah), but I'm not afraid of aggressively refactoring this later down the line once I set up the test suite for this and any potential regression tests. I'm prioritizing development speed over beauty for the time being.

@vhyrro
Copy link
Contributor Author

vhyrro commented Jun 29, 2024

Waiting for the CI to complete and I believe we can merge :)

@vhyrro vhyrro merged commit a9e78f9 into master Jun 29, 2024
2 of 3 checks passed
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.

3 participants