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

move inchi-1 specific functions out of library #55

Open
wants to merge 1 commit into
base: rwth
Choose a base branch
from

Conversation

giallu
Copy link
Contributor

@giallu giallu commented Sep 9, 2024

PrintFileName and SplitTime are compiled in the library only when TARGET_API_LIB is not defined, but that is never the case in the cmake build so it fails.
Apparently, those two symbols and few others like DetectInputINChIFileType are only used in the inchi-1 executable, so they were moved in ichimain.c

Please note, this raises a few questions about the whole build system (please let me know if it's better to discuss this elsewhere). In particular, it seems the original Makefiles sort of build a slightly different library depending on the target being built; this makes things a bit confusing in the code and hard to grasp due to the various combinations possible.
Also, this is completely unsupported with cmake so I hope you are open to patches like the present one in order to gradually move toward removing them.

For the record, after this patch is applied there are 32 instances left of #ifndef TARGET_API_LIB that I need to investigate because that's code never built into the library, and 17 instances of the reverse check #ifdef TARGET_API_LIB that mostly seem to add some symbols to the library.

@giallu
Copy link
Contributor Author

giallu commented Sep 9, 2024

Please note, the CI action fails (at least in my fork) due to some recent changes in the build/test scripts that fails due to the giallu/ prefix in the branch's name...

@giallu
Copy link
Contributor Author

giallu commented Sep 9, 2024

The added commit fixes the tests by using SHA instead of branch name to set the library filename

@giallu giallu force-pushed the giallu/move_inchi-1_specific_functions branch from b00a827 to 0cf5cca Compare September 10, 2024 08:54
@giallu
Copy link
Contributor Author

giallu commented Sep 10, 2024

Sorry, had to force/push to remove an unrelated change that sneaked into the SHA commit

@giallu giallu force-pushed the giallu/move_inchi-1_specific_functions branch 2 times, most recently from 619c09b to 3031bf4 Compare September 19, 2024 09:36
@giallu giallu force-pushed the giallu/move_inchi-1_specific_functions branch from 3031bf4 to 934405f Compare September 27, 2024 23:30
@JanCBrammer JanCBrammer force-pushed the rwth branch 5 times, most recently from 1d99ad3 to cfee545 Compare October 18, 2024 07:19
@JanCBrammer JanCBrammer force-pushed the rwth branch 3 times, most recently from 3d933ef to fcd8c4c Compare October 31, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant