-
Notifications
You must be signed in to change notification settings - Fork 26
ghdl: use DESTDIR instead of --prefix #59
base: main
Are you sure you want to change the base?
Conversation
The default prefix of GHDL is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you merge the latest from the main
branch that CI error should be fixed now - it was unrelated to this.
Will this make it so that GHDL_PREFIX
doesn't need to be set? if so, please remove this line from the test scripts so we can verify https://github.com/open-tool-forge/fpga-toolchain/blob/071ccd2/scripts/test/install_toolchain.sh#L40
Also, the goal is to have the toolchain set up so hopefully it can be run from any prefix, not just from edit: See here for some more details I found about the ghdl search path #4 (comment). In summary, there is a list of search paths it goes through including relative to the binary. It works correctly on windows, for some reason it's broken on the other platforms |
The motivation for using However, that would not solve the issue with GHDL_PREFIX. This PR is just because "hardcoding" a path in the tools which is unlikely to exist feels weird. I do understand why you don't want to use What do you think about using
According to @tgingold (im-tomu/fomu-workshop#334 (comment)) those search paths are for the GHDL binary, not for libghdl or the plugin. Therefore, it is surprising that it works on Windows. However, I removed my MSYS2 installation of GHDL, and I can confirm that fpga-toolchain works ok without it. |
I'm not sure what you mean. When compiling ghdl and libghdl, the script runs
As far as I can tell, what happens on windows is that it looks in the configured search paths which are wrong so then it looks relative to
I think the behaviour on Windows is the ideal situation but given I wasn't able to replicate it on Linux and OS X that sounds like a good compromise. It seems less likely to clash with existing installations. Basically I just really want to make sure it doesn't end up loading vhdl libraries other than the ones bundled into this package. |
My first change was: ./configure
...
make DESTDIR=$PACKAGE_DIR/$NAME install But that placed the GHDL binary in ./configure --prefix=/
...
make DESTDIR=$PACKAGE_DIR/$NAME install Which is the current state of this PR. However, I agree it might produce conflicts.
That's a very interesting insight. Yes, let's wait for Tristan.
Let's see whether Tristan considers that behaviour (taking the location of the yosys binary) acceptable. If so, there is nothing to fix. Otherwise, we can go with A issue I see with that behaviour is that it might be problematic when the plugin is loaded dynamically. |
I'm manually re-running the CI build thanks to this bug >_< #61 |
As far as I can tell, what happens on windows is that it looks in the configured search paths which are wrong so then it looks relative to argv[0] which is the yosys binary rather than the ghdl binary. That works well (even if it's maybe unintended). For some reason that same API call returns NULL/empty string for argv[0] on Linux and OS X. I don't know if @tgingold would be willing to offer any insights?
A different API is used on Windows to get the process path. Maybe it is not a good idea for libghdl.
|
Ah, that explains the different behaviour then! I think I found the code you're talking about here https://github.com/ghdl/ghdl/blob/657fcfde5fb93c1311fe5fd2d28146c89852614d/dist/windows/mcode/windows_default_path.adb#L44 |
So, @tgingold, do you suggest that we enforce using |
Thinking about it more, I'd really prefer a solution that makes it simple to have multiple nightly builds installed at once without risk of "cross-contaminating" them by relying on an absolute search path. A
(the yosys binary would be renamed to _yosys) edit: we would probably need a similar wrapper script for ghdl too for consistency |
What about modifying the ghdl-yosys-plugin to use the same prefix as Yosys ? That would avoid the need of the wrapper script.
|
That sounds like a great solution! |
Agree, that'd be ideal, as it would make GHDL_PREFIX required only in specific development contexts. @edbordin if a wrapper was used, I believe it'd be easier to use Nevertheless, if this issue is fixed upstream, the only motivation for maintaining this PR is providing nicer errors. Shall I close it or are you ok with using |
@umarcor I think I'm happy to merge it with |
c4c992b
to
850266b
Compare
Currently,
compile_ghdl.sh
uses--prefix
for modifying the location wheremake install
places the artifacts. However, configuring GHDL with a custom prefix has other effects. It is hardcoded in the tool, and it is later used for searching libs. As a result, it can produce errors with weird paths, such as https://github.com/umarcor/fomu-workshop/runs/1243053586?check_suite_focus=true#step:9:46.In this PR, the prefix is not modified. Instead,
DESTDIR
is used for tellingmake install
where to put the content.Ref: https://github.com/ghdl/ghdl/blob/master/dist/ci-run.sh#L317