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: link LFortran with -I #44

Closed
wants to merge 1 commit into from

Conversation

Pranavchiku
Copy link
Member

With this SNAP works, the trouble is location information of variables in different modules. This PR now links -I every directory so that it finds mod files and link.

image

const flags = ["--show-document-symbols", "--continue-compilation"];
// run `echo "$(find . -type d | sed 's/^/-I/')"` and pass it as an argument to lfortran using spawnSync
// Execute the command within a shell
const find_output = spawnSync("echo $(find $(pwd) -type d | sed 's/^/-I/')", { shell: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this has several problems:

  • it will insert insane long command line arguments for large repos
  • it will have collision problems for projects that have unrelated projects with modules with the same name
  • it depends on find and sed, which is not available on all platforms (like Windows)


// generate .mod files for the module
const module_flags = ["-c", "--continue-compilation"];
const run_module = await this.runCompiler(settings, module_flags, text, "[]");
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean lfortran now runs twice every time for each key stroke?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it does.

@certik certik marked this pull request as draft December 20, 2024 17:27
@certik
Copy link
Contributor

certik commented Dec 20, 2024

This shows where the problem is and a quick prototype fix. Thanks for figuring it out.

However the right fix is to let the use specify the path via an option. Maybe we can add an option to automatically include all subdirectories, but it seems hackish.

Regarding regenerating of .mod files --- this comes to the recompilation strategy, and I think for now we should let the user compile via his build system manually, not try to recompile, because we will fail, since we do not know the proper compiler options for each file.

However. As discussed, we should save the proper compiler option into the .mod file, and then we probably can recompile automatically.

const module_flags = ["-c", "--continue-compilation"];
const run_module = await this.runCompiler(settings, module_flags, text, "[]");

this.logger.info(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be logged as a debug message instead of an info message. It will also be logged, already, if you set your log level to trace in the extension's settings.

@Pranavchiku
Copy link
Member Author

since we do not know the proper compiler options for each file.

Aah yes, makes sense.

@Pranavchiku
Copy link
Member Author

However the right fix is to let the use specify the path via an option.

Do you mean option in extension settings? or in LFortran?

@Pranavchiku
Copy link
Member Author

Pranavchiku commented Dec 21, 2024

I just cleaned the PR and kept it in a state which if we come up with no solution can be merged and get SNAP working on macos / linux. Now looking at incorrect location information of module files.

@Pranavchiku
Copy link
Member Author

With #45 and by providing -Isrc/ we can make SNAP work:

image

cc @certik

@certik
Copy link
Contributor

certik commented Dec 21, 2024

Perfect! Much better approach.

@dylon
Copy link
Collaborator

dylon commented Dec 23, 2024

With #45 and by providing -Isrc/ we can make SNAP work:

image cc @certik

Ah, this is what the feature flags are for. I prefer this approach, too. I'll go ahead and submit a PR against your other feature branch to make it use an array of strings instead of a single, space-separated string. Space-separated strings for multiple command arguments are risky for situations such as when a user wants to include a path with a non-standard character in its name, like a space character.

@certik
Copy link
Contributor

certik commented Dec 24, 2024

I think we can close this PR now, it's been implemented in other PRs.

@certik certik closed this Dec 24, 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