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

Include directory structure proposal. #1674

Merged
merged 1 commit into from Mar 7, 2021
Merged

Conversation

m-kru
Copy link
Contributor

@m-kru m-kru commented Mar 5, 2021

I would propose following policy for storing and installing include header files.

  1. They are always in separate include directory, for example in src/synth/include.
  2. In terms of the installation they are put under ghdl/. For exampple,
ghdl/
├── synth
│   ├── gates.h
│   └── synth.h
├── vhpi_user.h
└── vpi_user.h

The include would be #include "ghdl/synth/synth.h".

The Makefile probably can be improved, but this is just proposal.

@tgingold
Copy link
Member

tgingold commented Mar 5, 2021 via email

@m-kru
Copy link
Contributor Author

m-kru commented Mar 5, 2021

@tgingold no I don't. Should it be applied only here https://github.com/ghdl/ghdl-yosys-plugin?

@tgingold
Copy link
Member

tgingold commented Mar 5, 2021 via email

@m-kru
Copy link
Contributor Author

m-kru commented Mar 5, 2021

Ok, I think it would also make sense to do the same for vhpi and vpiif there are going to be more header files related with them.

Copy link
Member

@umarcor umarcor left a comment

Choose a reason for hiding this comment

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

IMHO the synth subdir adds unnecessary complexity. There are two header files and four total. Even if we add a couple more, having additional subdirs is overkill. Instead, I would merge synth.h and gates.h in a single file.

I think that renaming ghdlsynth to synth and ghdlsynth_gates to gates is sensible.

@m-kru
Copy link
Contributor Author

m-kru commented Mar 6, 2021

@umarcor
I agree, after some thoughts I think there is no gain in having separate directories within ghdl directory. It adds extra complexity to the Makefile and in C symbols are global anyway.

I would merge synth.h and gates.h in a single file.

I guess they are separate because one is auto generated.
Please note that gates.h is included in synth.h.
I would not merge them.

@umarcor
Copy link
Member

umarcor commented Mar 6, 2021

I guess they are separate because one is auto generated.
Please note that gates.h is included in synth.h.
I would not merge them.

I was thinking about concating them during install, in case you wanted to have a single unit (subdir or file) for synthesis. However, after your clarification, I agree with not merging.

@m-kru m-kru force-pushed the include_dir branch 2 times, most recently from 1d6c0a2 to 40c4f3f Compare March 7, 2021 07:36
@m-kru
Copy link
Contributor Author

m-kru commented Mar 7, 2021

How are the tests for vpi organized? If I replace #include <vpi_user.h> with #include <ghdl/vpi_user.h> they fail. If I do not replace they also fail.

@tgingold
Copy link
Member

tgingold commented Mar 7, 2021 via email

@m-kru m-kru force-pushed the include_dir branch 3 times, most recently from 3755160 to dad9b29 Compare March 7, 2021 08:42
@m-kru
Copy link
Contributor Author

m-kru commented Mar 7, 2021

If I understand correctly, another way is to add ghdl/ to install destination paths in

install.vpi.local: all.vpi
	$(MKDIR) -p include lib
	$(INSTALL_DATA) -p $(GRTSRCDIR)/vpi_user.h include/
	$(INSTALL_DATA) -p $(GRTSRCDIR)/vhpi_user.h include/

and then use #include <ghdl/vpi_user.h> in tests, the same way it is supposed to be used by the user.

@m-kru
Copy link
Contributor Author

m-kru commented Mar 7, 2021

But wait ..., if I was correct then the tests should pass right now.

@tgingold
Copy link
Member

tgingold commented Mar 7, 2021 via email

@m-kru
Copy link
Contributor Author

m-kru commented Mar 7, 2021

Ok, it is all getting a little bit fuzzy, so lets clear it.
There are 2 Makefile targets install.vpi and install.vpi.local. I am not sure what is the purpose of the second one?
I also do not understand what is the of point --vpi-include?

I think it is important to keep #include <vpi_user.h> so that the code is portable among simulators.

Isn't it related with what people have in PATH?
I really do not like installing headers to some common include paths like /usr/local/include.
It is like delay-action bomb.
Some simulators will have to update #include <vpi_user.h> to #include <ghdl/vpi_user.h>, but this is very small change.

@tgingold
Copy link
Member

tgingold commented Mar 7, 2021 via email

@m-kru
Copy link
Contributor Author

m-kru commented Mar 7, 2021

Ok, I I understand what you have written, but I do not understand why is this complexity with --vpi-include-dir needed.

In *nix there are some paths in the include PATH by default, for example /usr/local/include.
If some tool ABC wants to install headers in one of the default paths there are 2 possible ways (there are of course more, but these are the rule of the thumb).

  1. If this is single header, then name it ABC.h and install it directly in the /usr/local/include.
  2. If these are multiple header files, then create directory ABC within /usr/local/include and put headers into that directory. /usr/local/include/ABC/foo.h, /usr/local/include/ABC/bar.h.

If user wants some custom installation path then it is specified in the Makefile with variable or during configure step.
However, GHDL (the tool) is aware of the vpi include directory, and user can "ask" it to get that directory path.
I am sure it serves some purpose, however at first glance it looks very suspicious, like something was structured in a intricate way.

@tgingold
Copy link
Member

tgingold commented Mar 7, 2021 via email

@m-kru
Copy link
Contributor Author

m-kru commented Mar 7, 2021

Ok, the tests pass. The only change that would need to be applied to the ghdl-yosys-plugin is probably:

diff --git a/src/ghdl.cc b/src/ghdl.cc
index 413d371..de06f1f 100644
--- a/src/ghdl.cc
+++ b/src/ghdl.cc
@@ -27,7 +27,7 @@ USING_YOSYS_NAMESPACE

 #ifdef YOSYS_ENABLE_GHDL

-#include "ghdlsynth.h"
+#include "ghdl/synth.h"

 using namespace GhdlSynth;

@tgingold you can check and merge if you like, or give feedback what you do not like.

@Xiretza
Copy link
Contributor

Xiretza commented Mar 7, 2021

I think install.vpi.local needs to be adjusted (or the include path setup for tests should be reworked completely), currently tests fail because they can't find the header if ghdl is not installed.

@m-kru
Copy link
Contributor Author

m-kru commented Mar 7, 2021

@Xiretza which tests?

@Xiretza
Copy link
Contributor

Xiretza commented Mar 7, 2021

gna issue1226 is the first one that fails, but all VPI tests should be affected. ghdl --vpi-compile adds -I/path_to_ghdl_repo/include/ghdl to the compiler arguments, which doesn't exist.

@m-kru
Copy link
Contributor Author

m-kru commented Mar 7, 2021

@Xiretza where are these tests? Why are they not catched by the CI?

@Xiretza
Copy link
Contributor

Xiretza commented Mar 7, 2021

This specific one is in testsuite/gna/issue1226/, and it's not caught because the CI runs the tests in a docker image where ghdl is already installed, which is honestly pretty unconventional. Maybe that should be changed, but that's mostly an unrelated issue. For now, install.vpi.local should probably just install the headers to include/ghdl/ instead of include/.

@m-kru
Copy link
Contributor Author

m-kru commented Mar 7, 2021

Ok this is all getting messy. I would opt for one uniform way. Locally or globally headers are always placed in .../include/ghdl/ directory. However, the include path returned by GHDL or added with --vpi-compile should be ../include, and within C sources there should always be #include <ghdl/foo.h>.

@Xiretza
Copy link
Contributor

Xiretza commented Mar 7, 2021

I think #include <vpi_user.h> is expected to work as-is, without a tool prefix - so either --vpi-compile has to include the ghdl/ directory in the include path, or the header needs to be placed directly in include/, outside the new directory structure.

@m-kru
Copy link
Contributor Author

m-kru commented Mar 7, 2021

Why ghdl --vpi-compile serves as an wrapper for vpi compilation at all?

If it is supposed that user will always compile vpi via ghdl --vpi-compile and never directly via gcc or clang, then why is it installed in one of the system default include paths at all?
If it is intended to be used anyhow user wants, then it should be placed in include/ghdl. I know it breaks backward compatibility, but it will be done how it should be done from the beginning. By the way, project of any size can be easily adjusted to this change within few seconds:

find . -name *.c -exec sed -i 's/#include <vpi_user.h>/#include <ghdl\/vpi_user.h>/' {} \;

.

@Xiretza
Copy link
Contributor

Xiretza commented Mar 7, 2021

vpi_user.h is a standard header and not specific to GHDL, so it doesn't really make much sense to install it under a ghdl prefix and force people to either add it to their include path or specialize all the source files to GHDL.

@tgingold tgingold merged commit 66cd5e0 into ghdl:master Mar 7, 2021
@tgingold
Copy link
Member

tgingold commented Mar 7, 2021

Thanks. I will fix the fallouts.

@tgingold
Copy link
Member

tgingold commented Mar 7, 2021 via email

@tgingold
Copy link
Member

tgingold commented Mar 7, 2021 via email

tgingold added a commit that referenced this pull request Mar 7, 2021
@m-kru m-kru deleted the include_dir branch March 8, 2021 06:23
@umarcor umarcor added Build: Makefile Building GHDL using makefiles (standard Linux, Unix, MacOS environments) Enhancement Feature: Synthesis VHDL to netlist transformation. Testsuite: vhpi Testsuite: vpi labels Mar 8, 2021
@umarcor umarcor added this to the v2.0 milestone Mar 8, 2021
@eine
Copy link
Collaborator

eine commented Mar 8, 2021

This specific one is in testsuite/gna/issue1226/, and it's not caught because the CI runs the tests in a docker image where ghdl is already installed, which is honestly pretty unconventional.

GHDL's testsuite was initially designed for being executed after make install. Installing GHDL in the same environment where it was built did lead to dependency issues. This is because build dependencies are typically a superset of runtime dependencies. Therefore, in CI, GHDL is built on one environment and it is tested on a different environment. On Linux, containers are used. On Windows, different jobs are used. IMHO, what we do unconventionally is using tarballs instead of DEB/RPM packages on Linux. Using containers is the only solution for testing multiple distributions (Debian, Ubuntu, Fedora and Arch).

Later, some features were added to the makefiles because some packagers wanted to run the testsuite in-place and before make install. I consider that to be a partial/smoke testing, not a robust execution of the testsuites.

umarcor pushed a commit to umarcor/ghdl that referenced this pull request Aug 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build: Makefile Building GHDL using makefiles (standard Linux, Unix, MacOS environments) Enhancement Feature: Synthesis VHDL to netlist transformation. Testsuite: vhpi Testsuite: vpi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants