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

File access #1715

Merged
merged 19 commits into from
Apr 28, 2023
Merged

File access #1715

merged 19 commits into from
Apr 28, 2023

Conversation

edwardalee
Copy link
Collaborator

@edwardalee edwardalee commented Apr 21, 2023

This is a companion to the file-access PR in reactor-c. It adds definitions for LF_SOURCE_DIRECTORY and LF_FILE_SEPARATOR to the generated cmake file to enable programs to read files located relative to the source .lf file.

With these changes, the Rhythm examples in the examples repo work again, at least on MacOS. It would be great if someone could test them on Linux.

@edwardalee edwardalee requested a review from cmnrd April 21, 2023 23:10
@edwardalee edwardalee added the enhancement Enhancement of existing feature label Apr 21, 2023
Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

The changes here look good to me. I have not yet tested the Rythm example though. However, instead of testing an example once, I think we should add integration tests for this feature.

@edwardalee
Copy link
Collaborator Author

I have added tests for both federated and unfederated versions. To get the federated test working, I had to resurrect the lf_set_array function, which is documented on the website but had somehow disappeared.

I believe this is ready to merge, modulo tests passing.

Copy link
Collaborator

@petervdonovan petervdonovan left a comment

Choose a reason for hiding this comment

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

to enable programs to read files located relative to the source .lf file.

Just a thought (not a conviction) -- Would it be preferable to have users read relative to the project root? This seems like a more popular choice among other programming languages, maybe to avoid long prefixes of ../../../ and to avoid having to change imports when the file that does the imports is moved.

We could also expose both paths.

@edwardalee
Copy link
Collaborator Author

to enable programs to read files located relative to the source .lf file.

Just a thought (not a conviction) -- Would it be preferable to have users read relative to the project root? This seems like a more popular choice among other programming languages, maybe to avoid long prefixes of ../../../ and to avoid having to change imports when the file that does the imports is moved.

We could also expose both paths.

I guess this would be FileConfig.outPath? I'm not sure. The documentation doesn't claim this to be a project root. Doesn't the notion of a project root depend on the IDE?

@lhstrh
Copy link
Member

lhstrh commented Apr 26, 2023

I agree with @petervdonovan, and we have also discussed this previously. Yes, it should be possible to look relative to the file, but we should also be able (and encourage, for the above-stated reasons) to look relative to the project root. The project root is the parent directory of the src directory. Right now, this is just a convention, but as soon as people get used to creating project using lingo init this will be much clearer. I have another PR in the works that fixes the files target property and does a look up relative to the project root as well. I am yet to figure out how that relates to this PR.

@lhstrh
Copy link
Member

lhstrh commented Apr 26, 2023

I think that LF_SOURCE_DIRECTORY is ambiguous because there can be many source files that are part of a single LF program. Perhaps it should be LF_MAIN_SRC_DIR or something like this? We could additionally have a LF_PROJECT_DIR to enable look ups relative to the project root.

@edwardalee
Copy link
Collaborator Author

I think that LF_SOURCE_DIRECTORY is ambiguous because there can be many source files that are part of a single LF program. Perhaps it should be LF_MAIN_SRC_DIR or something like this? We could additionally have a LF_PROJECT_DIR to enable look ups relative to the project root.

This sounds good to me. Can you clarify for me which FileConfig fields correspond to these?

@lhstrh
Copy link
Member

lhstrh commented Apr 26, 2023

Looks like it should be srcPkgPath.

@edwardalee
Copy link
Collaborator Author

Looks like it should be srcPkgPath.

Hmm. That says "This path is determined differently depending on whether the compiler is invoked through the IDE or from the command line." This means that the same program will work or fail depending on how the compiler is invoked. I think I will stick with the original design.

@lhstrh
Copy link
Member

lhstrh commented Apr 27, 2023

Looks like it should be srcPkgPath.

Hmm. That says "This path is determined differently depending on whether the compiler is invoked through the IDE or from the command line." This means that the same program will work or fail depending on how the compiler is invoked. I think I will stick with the original design.

It's really just the parent of the src directory that is the root of the project. This is not hard to find and is probably already computed in FileConfig. Let me check again.

@edwardalee
Copy link
Collaborator Author

Looks like it should be srcPkgPath.

Hmm. That says "This path is determined differently depending on whether the compiler is invoked through the IDE or from the command line." This means that the same program will work or fail depending on how the compiler is invoked. I think I will stick with the original design.

It's really just the parent of the src directory that is the root of the project. This is not hard to find and is probably already computed in FileConfig. Let me check again.

In Epoch, at least, the bin and src-gen directories go into a different place depending on where you set the root of the project. I have a project that includes all tests for all targets. It puts bin in lingua-franca/test, not lingua-franca/test/C, as the command-line does.

@lhstrh
Copy link
Member

lhstrh commented Apr 27, 2023

In Epoch, at least, the bin and src-gen directories go into a different place depending on where you set the root of the project. I have a project that includes all tests for all targets. It puts bin in lingua-franca/test, not lingua-franca/test/C, as the command-line does.

I see. This makes it ambiguous as to what the project root is. IMO, a package layout where the src directory is in some subdirectory structure is a misconfigured package. More important than the src directory will be the presence of a Lingo.toml file that describes the package. We don't have that now, but we will have it soon.

@lhstrh
Copy link
Member

lhstrh commented Apr 28, 2023

How about we change the behavior slightly to get more uniformity? Let's determine the package root the same way for all environments and not treat Epoch specially. The only situation in which, I think, it makes sense to do so, is when there is no src directory or Lingo.toml. In that case, for lfc we use the current working directory as the package root, and for Epoch we use the workspace root.

@edwardalee
Copy link
Collaborator Author

Sounds good to me.

@edwardalee edwardalee merged commit 922ed3f into master Apr 28, 2023
@edwardalee
Copy link
Collaborator Author

Finally got Windows to work! Merging now with main, and will add project root option as a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants