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.

Sign up for GitHub

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

Jump to bottom

fix(build/builtin): properly auto-detect modules #238

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(build/builtin): properly auto-detect modules #238

wants to merge 1 commit into from

Conversation

mrcjkb
Copy link
Member

@mrcjkb mrcjkb commented Dec 4, 2024

Fixes #236.

@mrcjkb
Copy link
Member Author

mrcjkb commented Dec 4, 2024

Actually, this was just a bug in LuaModule::from_pathbuf. No need to autodetect them in the install step.

I've also moved the build.utils module from rockspec.build.utils to build.utils.

@mrcjkb
Copy link
Member Author

mrcjkb commented Dec 4, 2024 •
edited
Loading

  • We should also add a test case.

@mrcjkb mrcjkb force-pushed the mj/push-wwowvvnmupyy branch from 70a1be4 to d8f8ee0 Compare December 4, 2024 06:56
@mrcjkb mrcjkb marked this pull request as draft December 4, 2024 06:56
@mrcjkb mrcjkb force-pushed the mj/push-wwowvvnmupyy branch 2 times, most recently from 6319d9a to 90c886b Compare December 4, 2024 07:12
@mrcjkb mrcjkb changed the title fix(install): auto-detect modules fix(build): auto-detect modules Dec 4, 2024
@mrcjkb mrcjkb force-pushed the mj/push-wwowvvnmupyy branch 2 times, most recently from 37bc52a to 0a91129 Compare December 4, 2024 07:13
@mrcjkb mrcjkb changed the title fix(build): auto-detect modules fix(build): auto-detect modules broken Dec 4, 2024
@mrcjkb mrcjkb force-pushed the mj/push-wwowvvnmupyy branch from 0a91129 to 83304bf Compare December 4, 2024 16:01
@mrcjkb mrcjkb changed the title fix(build): auto-detect modules broken fix(build/builtin): properly autodetect modules Dec 4, 2024
@mrcjkb mrcjkb marked this pull request as ready for review December 4, 2024 16:03
@mrcjkb mrcjkb force-pushed the mj/push-wwowvvnmupyy branch from 83304bf to 6f36aad Compare December 4, 2024 16:03
@mrcjkb mrcjkb marked this pull request as draft December 4, 2024 16:17
@mrcjkb mrcjkb force-pushed the mj/push-wwowvvnmupyy branch from 6f36aad to 8c83bc8 Compare December 4, 2024 21:00
@mrcjkb mrcjkb marked this pull request as ready for review December 4, 2024 21:03
@mrcjkb mrcjkb force-pushed the mj/push-wwowvvnmupyy branch 2 times, most recently from daec215 to b521dcb Compare December 4, 2024 23:12
@mrcjkb mrcjkb mentioned this pull request Dec 4, 2024
Open
@mrcjkb mrcjkb force-pushed the mj/push-wwowvvnmupyy branch from b521dcb to 5b3b7b7 Compare December 4, 2024 23:22
@mrcjkb mrcjkb mentioned this pull request Dec 4, 2024
Open
@mrcjkb mrcjkb force-pushed the mj/push-wwowvvnmupyy branch from 5b3b7b7 to 13b5367 Compare December 4, 2024 23:44
@mrcjkb
Copy link
Member Author

mrcjkb commented Dec 4, 2024

Turns out we should only auto detect modules if no modules are specified (for example, successfully auto detecting modules breaks luassert).

@mrcjkb mrcjkb changed the title fix(build/builtin): properly autodetect modules fix(build/builtin): properly auto-detect modules Dec 4, 2024
@mrcjkb mrcjkb force-pushed the mj/push-wwowvvnmupyy branch from 13b5367 to e832971 Compare December 10, 2024 19:42
vhyrro
vhyrro reviewed Dec 11, 2024
View reviewed changes
rocks-lib/src/build/builtin.rs
.into_iter()
.chain(self.modules)
.collect::<HashMap<_, _>>();
let modules = if self.modules.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

question(non-blocking): is this luarocks's behaviour too? Is there no situation where a few modules are defined by the rockspec author and they "expect" the rest to be autodetected? Given the amount of wacky code we've seen it might be worth checking haha

Copy link
Member Author

@mrcjkb mrcjkb Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure.
I tried installing luassert and it failed if using both the rockspec modules and the auto-detected modules.
But perhaps instead we should just filter out files in the rockspec modules when auto-detecting modules.

rocks-lib/Cargo.toml
@@ -49,6 +49,7 @@ thiserror = "2.0.0"
gpgme = "0.11.0"
futures = "0.3.31"
async-recursion = "1.1.1"
predicates = "3.1.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: I only see this used in tests. Should this be part of dev-dependencies instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vhyrro vhyrro vhyrro left review comments

@NTBBloodbath NTBBloodbath Awaiting requested review from NTBBloodbath NTBBloodbath is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] auto detected lua modules are not installed if not specified in the install spec
2 participants
@mrcjkb @vhyrro