Update Foreign.rst to reflect my learnings #1195
Conversation
I found the page to be a little unclear in when and why to link to externally generated object files. It is a simple point in hindsight, but moving Linking foreign object files to GHDL to right after Foreign Declarations of Function makes it easier to see the link. I also added a Hint on how to pass command line arguments to ghdl_main, and wrote that the one code block is Python code (which wasn't immediately obvious to me at least).
| @@ -62,6 +62,25 @@ The value of the attribute must start with ``VHPIDIRECT`` (an | |||
| upper-case keyword followed by one or more blanks). The linkage name of the | |||
| subprogram follows. | |||
|
|
|||
| The object file with the source code for the foreign subprogram must then be | |||
There was a problem hiding this comment.
I don't think it's possible to rellocate this content here. The hierarchy of the content is:
- Foreign declarations
- Restrictions on foreign declarations
- Linking foreign object files to GHDL
- Wrapping and starting a GHDL simulation from a foreign program
- ...
You are changing it to:
- Foreign declarations
- Linking foreign object files to GHDL
- Restrictions on foreign declarations
- Wrapping and starting a GHDL simulation from a foreign program
- ...
"Linking..." needs to be either before "Foreign declarations" or after "Restrictions on foreign declaration".
doc/using/Foreign.rst
Outdated
| @@ -140,6 +143,9 @@ in C: | |||
|
|
|||
| extern int ghdl_main (int argc, char **argv); | |||
|
|
|||
| .. HINT:: | |||
There was a problem hiding this comment.
Strictly, it should not be an unused entry. Instead, it should be the path of the executed binary. Hence, I would not suggest to let it empty by default; we should explain two options:
- Pass/reuse the first argument that was passed to
main, whereghdl_mainis called from. - Leave it empty/null.
Then, it is up to the user to decide.
Instead of adding a reference to a comment in some issue, I'd invite you to copy the relevant content from that comment to the docs. Actually, how to pass CLI arguments to GHDL might need to be clarified in a separate section, instead of being just a HINT. Relatively recent related issues: #1066, #819, #607.
Other meaningful references are https://ghdl.readthedocs.io/en/latest/using/InvokingGHDL.html#elaborate-and-run-elab-run (note elab_options and run_options) and https://ghdl.readthedocs.io/en/latest/using/Simulation.html (maybe "Simulation options" should be renamed to "Run options", to make it easier to understand that this is where run_options are defined).
So, the bare minimum rewrite would be to include references to those two sections in this HINT. From there on, up to the effort you want/can put on it.
There was a problem hiding this comment.
Sure. I have expanded on when to elaborate with the main.o.
maybe "Simulation options" should be renamed to "Run options"
I saw that Simulation and Runtime is the actual index. Even so, I was confused when trying to figure when I could use --disp-tree, namely that it did not work with --elab-run and it wasn't clear that it was a -r option. In the docs nor in the errors from ghdl. I think the examples we keep talking about would clear that up.
There was a problem hiding this comment.
how to pass CLI arguments to GHDL might need to be clarified in a separate section
I left it in a hint. At most, it (CLI arguments via a custom main() to ghdl_main) could just be a sub under Wrapping and starting a GHDL simulation from a foreign program. I feel another revision coming on.
There was a problem hiding this comment.
Sure. I have expanded on when to elaborate with the main.o.
I'm not sure about the location of the first hint you added... It feels like it would better fit as a rewrite/clarification of https://ghdl-rad.readthedocs.io/en/latest/using/Foreign.html#linking-foreign-object-files-to-ghdl. Then, https://ghdl-rad.readthedocs.io/en/latest/using/Foreign.html#wrapping-and-starting-a-ghdl-simulation-from-a-foreign-program would be specific about how to call ghdl_main explicitly. The build procedure remains the same as in the previous section. Should anyone jump here directly, you can add a first sentence suggesting the previous section to be read.
From there on, things start to get slightly complicated. The next section (https://ghdl-rad.readthedocs.io/en/latest/using/Foreign.html#linking-ghdl-to-ada-c) is about how to call GCC instead of ghdl -e. However, the "knowledge" of the two previous sections apply. Then, the build procedure that is required for https://ghdl-rad.readthedocs.io/en/latest/using/Foreign.html#dynamically-loading-ghdl is the same as in https://ghdl-rad.readthedocs.io/en/latest/using/Foreign.html#linking-ghdl-to-ada-c, but with -shared. In between, we find https://ghdl-rad.readthedocs.io/en/latest/using/Foreign.html#dynamically-loading-foreign-objects-from-ghdl, which might make sense to be moved below. To make thins more fun, Linking foreign object files to GHDL, Wrapping and starting a GHDL simulation from a foreign program, Dynamically loading foreign objects from GHDL and Dynamically loading GHDL can be used all at the same time. AFAIK, none of the methods are mutually exclusive.
Last, https://ghdl-rad.readthedocs.io/en/latest/using/Foreign.html#using-grt-from-ada is a kind of separate category. It is about interacting with the "runtime kernel". It would be interesting to extend it to C and/or Python, but I'm not aware of any work in progress in this regard.
Overall, we are not native english speakers, so any touch you can give to the page as a whole in order to make it easier to read will be really welcome. It does not need to be all in a single PR, tho. To have this merged, I think it is ok to just explain the first hint in the previous section.
I saw that Simulation and Runtime is the actual index. Even so, I was confused when trying to figure when I could use --disp-tree, namely that it did not work with --elab-run and it wasn't clear that it was a -r option. In the docs nor in the errors from ghdl. I think the examples we keep talking about would clear that up.
I left it in a hint. At most, it (CLI arguments via a custom main() to ghdl_main) could just be a sub under Wrapping and starting a GHDL simulation from a foreign program. I feel another revision coming on.
Let's discuss these in #1059, so we keep this PR simple.
There was a problem hiding this comment.
To have this merged, I think it is ok to just explain the first hint in the previous section.
I did. I made it more generic too. It was a fair point you made. Thanks for your time
Move Linking... back to original spot. Better hint, and extra reference to simulation options.
Clean up reference hint.
Further clean reference hints.
Improve hints.
|
Oops. A little obvious how clumsy my .rst attempts are. I just edited in github which meant each commit has been verified. I assume not the best practice? |
|
It also makes keeping track of the overall change difficult. What is one supposed to do. branch the branch the merge when final? Or actually work locally then push. next time! |
It's ok. It can be easily squashed when the PR is accepted/merged. However, you have some alternatives not to reflect each tiny change here:
|
|
I'd like to keep the discussion on adding my hints here. I'll minimise how much I mention our other discussions. I moved the hint about -Wl,*.o listing to Linking foreign object files, as it does expand on the math library example. I made it more generic (instead of just talking about main.o). Then I refered to it in the place that it used to be (under wrapping and starting GHDL from a foreign prog). I want to move the hint about imitating run time flags to the added example that I mentioned in the above comment (here it is again (I didn't last that long...)) That really cleans up my additions. If we want to go further, I felt like the sections were a little confusing:
Then I would only discuss the order of the sections. But I need to find the best way I see to order them. So for now, I'm done. Thanks again! |
I think that it was good... anyway, you/we can recover it later. It is up to you.
Vraiment, ma langue est l'espagnol 😄, mais je sais lire et mal écrire en français.
Yes, it is alternative to compiling VHDL and other objects together. I think that You might have changed the line ending, because all the page is diffed now... |
|
@RocketRoss, see https://github.com/umarcor/ghdl/tree/doc-foreign, which is a single commit ahead of this PR. Let me know what you think. |
|
Your branch has everything I wanted, and more. I appreciate the help. We'll close this and then pull yours? |
In fact, the main purpose of this PR was for us to do a collaboration exercise. I think it's fair that you have this PR merged, and I will rebase and open a new one afterwards. Indeed... @tgingold, I believe this is ready for squash & merge! Please go ahead! |
|
Thanks! |
From #1185:
I found the page to be a little unclear in when and why to link to externally generated object files. It is a simple point in hindsight, but moving Linking foreign object files to GHDL to right after Foreign Declarations of Function makes it easier to see the link.
I also added a Hint on how to pass command line arguments to ghdl_main, and wrote that the one code block is Python code (which wasn't immediately obvious to me at least).
Description
Documentation clarity.
🚨 Before submitting your PR, please read contribute in the Docs, and review the following checklist:
When contributing to the docs...
Built here
❤️ Thank you!