base: add support for Pmac module. #26
Conversation
There was a problem hiding this comment.
As we talked in person, pmac may be installed with install_github_module (or even with download_github_module and install_module). Refactor to simplify things when #27 is merged.
I've found some points to improve nevertheless. You may work on them when refactoring changes.
There was a problem hiding this comment.
I don't think you need to specify the version in your commit message.
I'm missing an explanation in the message about the linux-x86_64 configuration files we removed. Explaining why we used --variable=includedir instead of --cflags-only-I may also be mentioned.
Indicating that the compilation error is probably due to not well-defined dependencies in the module's Makefile might be worth it to make this detail clearer.
base/install_motor.sh
Outdated
|
|
||
| SSH = $(pkg-config --variable=prefix libssh2) | ||
| SSH_LIB = $(pkg-config --variable=libdir libssh2) | ||
| SSH_INCLUDE = -I$(pkg-config --variable=includedir libssh2) |
There was a problem hiding this comment.
@ericonr, do you have any clue why --cflags-only-I wouldn't work even with variables defined in libssh2.pc?
I've compared the syntax along with @guirodrigueslima, but I couldn't understand what was happening. In this case, it is okay to have includedir, since it has a single include directory.
There was a problem hiding this comment.
I will try to take a look at the pkg-config issue. It's likely that the package is available in the default includedir, so there's no need to actually define any cflag for it.
EDIT: that was just it. Does the build explicitly require the INCLUDES variable be defined?
I believe this PR is missing documentation update (in the README) mentioning the new libssh2 dependency for IOCs using the Pmac module.
If possible, formatting the commit message body to fit in 72 columns would also be nice (can be done inside Vim in git commit --amend dialog by selecting the body and pressing gq). Otherwise I can do it myself before merging.
There was a problem hiding this comment.
I was almost forgetting, but since v0.4.0 we should add an entry in CHANGES.md under Unreleased section indicating this PR under "New features".
No, it doesn't. It only appends its contents to On the other hand, |
README.md
Outdated
| `Pmac` IOCs must include `libssh2-1` in `RUNTIME PACKAGES` at build, | ||
| because they are not integrated. |
There was a problem hiding this comment.
| `Pmac` IOCs must include `libssh2-1` in `RUNTIME PACKAGES` at build, | |
| because they are not integrated. | |
| `Pmac` IOCs must include `libssh2-1` in the `RUNTIME PACKAGES`, because the module depends on it. |
base/install_motor.sh
Outdated
|
|
||
| SSH = $(pkg-config --variable=prefix libssh2) | ||
| SSH_LIB = $(pkg-config --variable=libdir libssh2) | ||
| SSH_INCLUDE = -I$(pkg-config --variable=includedir libssh2) |
There was a problem hiding this comment.
In this case I'd either leave it as pkg-config --cflags-only-I libssh2 (the empty output might change on some other distro? who knows) or empty.
There was a problem hiding this comment.
Agreed. Having it set by pkg-config seems better for me.
base/install_motor.sh
Outdated
| BUILD_IOCS=NO | ||
| USE_GRAPHICSMAGICK=NO | ||
|
|
||
| SSH = $(pkg-config --variable=prefix libssh2) |
There was a problem hiding this comment.
And consider making this SSH=YES
There was a problem hiding this comment.
I've thought about that too. We should just remember that SSH=NO doesn't mean what it looks like =P.
Compilation error in Pmac module with JOBS>1, probably due to badly defined dependencies in the module's Makefile. We set JOBS=1 to be able to build the module.
No description provided.