Conversation
JanCBrammer
left a comment
There was a problem hiding this comment.
I tested the steps "Build InChI" (line 45 in build.yml) followed by "Run executable tests) (line 67) locally in the devcontainer. I needed to install ninja-build in the devcontainer for the compilation to work. I.e., we should install ninja-build along with the other test dependencies:
Line 3 in c1435ac
The artifacts (executable and shared library) are built successfully and the tests pass.
I can also confirm that all artifacts (executable and shared library) have been uploaded from the latest workflow run: https://github.com/giallu/InChI/actions/runs/12657834492.
As an aside, I rebased build_matrix onto the latest main locally and didn't encounter any issues.
@djb-rwth, are there any compilation behaviors that are configured in the current makefiles, which are not reproduced by CMake? If so, and if we want to preserve those behaviors, how can we reproduce them with CMake?
| *.egg-info/ | ||
| # Ignore core dump files | ||
| core.*[0-9] | ||
| gcc_crash_report.txt |
There was a problem hiding this comment.
gcc_crash_report.txt used to be on main at the time build_matrix was branched off. It has since been removed.
|
|
||
| project( | ||
| inchi | ||
| VERSION 1.07 |
There was a problem hiding this comment.
We have to think about how to parametrize the version. Preferably we would have a single source of truth for the version. But maybe outside of this PR's scope.
There was a problem hiding this comment.
I agree on the single source for the version string; I guess it is possible to use several approaches, depending on how the release is handled: it can be read from a file, an env variable, from a workflow context variable, etc.
I would just avoid putting it in the sources, it's harder to extract for other tools and usually harder to find.
| add_subdirectory( INCHI-1-SRC/INCHI_API/demos/test_ixa/src ) | ||
| add_subdirectory( INCHI-1-SRC/INCHI_EXE/inchi-1/src ) | ||
|
|
||
| add_custom_target( run-tests |
There was a problem hiding this comment.
I will add a target for running the tests against the shared library as well.
Since this will require some refactoring of the test environment I'd prefer to do so in another PR in order to not bloat the present one.
|
Hi @giallu, I would like to like to draw your attention to some very important issues:
This will require a review of this PR, or even creating a new PR if many changes are needed. You are more than welcome to participate in creating, reviewing and/or testing this revised |
6d97b2d to
fb1f8b1
Compare
|
Thanks @djb-rwth for the review. My remarks follow:
While in theory you can reproduce 1:1 the current situation, I wouldn't recommend it for at least two reasons:
That commit was part of another PR #55, and is needed otherwise the build fails (see details in the other PR).
Cmake maintains a list of supported features and standards for each compiler. It's usually a good idea to leave to cmake the work of deciding which flags to pass to compilers and linkers, unless absolutely necessary; however, if the project wants to maintain specific settings for certain combinations of OS/compilers it is possible using the Presets feature Since it does not directly affect the topic of this PR I suggest to discuss it in a new issue to be opened after the merge.
I think this is already working this way |
|
Hi @giallu,
I am not convinced about this. Many users would just probably like to have an alternative build system to
At this moment, users can easily open a
I do not see the reason why would there be any other library except (shared) Adding a "wrapper"
I have already started to build a separate Various users have reported that parts of the
With all the above in mind, I think that might be a good idea in the foreseeable future.
The default Fortunately, new versions of
I agree.
According to this PR, the shared |
2fb8edb to
d863ffc
Compare
This commits add a GH workflow to build and run tests in several (Ubuntu 22.04, Windows 2019, OSX) operating systems at once, including Alpine (build only) and gcc-latest image.
Some notes:
port_to_cmakebranch, so we can work on this and close Move build system to cmake #37