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

Read non-hoisted deps #84

Closed

Conversation

DZakh
Copy link
Contributor

@DZakh DZakh commented Jan 12, 2024

It's not ready yet

Need to fix the error:

image

let path_buf =
match PathBuf::from(format!("{}/node_modules/{}", parent_path, package_name)).canonicalize() {
Ok(p) => p,
Err(_) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we print the error as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

match PathBuf::from(format!("{}/node_modules/{}", parent_path, package_name)).canonicalize() {
Ok(p) => p,
Err(_) => {
match PathBuf::from(format!("node_modules/{}", package_name)).canonicalize() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should use the absolute path and prepend with project_root here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

);
std::process::exit(2)
let path_buf =
match PathBuf::from(format!("{}/node_modules/{}", parent_path, package_name)).canonicalize() {
Copy link
Collaborator

@jfrolich jfrolich Jan 12, 2024

Choose a reason for hiding this comment

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

I think the is_root condition isn't handled here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not needed here, since the code never runs for the root package

Copy link
Collaborator

Choose a reason for hiding this comment

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

how is the root package handled then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok so it's only run for deps, so you are probably right :)

Copy link
Contributor Author

@DZakh DZakh left a comment

Choose a reason for hiding this comment

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

In a nutshell this PR moves does 3 things:

  1. Moves the code from helpers module to the Package impl
  2. Updates the logic to resolve the package path to support both hoisted and non-hoisted set up
  3. Starts using the resolved package path instead of building the hoisted path everytime it's needed

What's left:

  1. Update the last place to use resolved package path (in the compile.rs)
  2. Maybe use non-canonicalize paths for compiler errors ???
  3. Continue fixing next errors. After this PR it started finding files correctly, but I got another compilation error.
image

Comment on lines +369 to +370
// TODO: Add support for non-hoisted set up
let hoisted_path = format!("{}/node_modules/{}/lib/ocaml", root_path, &x);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to fix it in a separate PR

Comment on lines +94 to +100
pub fn get_ast_path(&self, source_file: &str) -> String {
helpers::get_compiler_asset(self, source_file, &packages::Namespace::NoNamespace, "ast")
}

pub fn get_iast_path(&self, source_file: &str) -> String {
helpers::get_compiler_asset(self, source_file, &packages::Namespace::NoNamespace, "iast")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it correct that you use packages::Namespace::NoNamespace here? Shouldn't it be package.namespace instead? Then, it would be able to move more code from helpers to the package implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes because asts never have namespaces in the filename

@DZakh DZakh requested a review from jfrolich January 15, 2024 10:58
@DZakh
Copy link
Contributor Author

DZakh commented Jan 17, 2024

I think I need to redo the PR from scratch. The conflicts are scarry 🙈

@DZakh
Copy link
Contributor Author

DZakh commented Feb 8, 2024

I'll try to find some time during the weekend to continue on the PR

@tsnobip
Copy link

tsnobip commented Feb 22, 2024

what's the status of this PR? Did you have time to work on it again @DZakh ? The current status makes it a blocker for my setup and at the same time, I'm really getting fed up with stale builds.

Is there a way I could help?

@DZakh
Copy link
Contributor Author

DZakh commented Feb 22, 2024

Sorry, had a lot of traveling recently. I'll have time to continue working on it this weekend.

@DZakh
Copy link
Contributor Author

DZakh commented Feb 25, 2024

Closed in favor of #87

@DZakh DZakh closed this Feb 25, 2024
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