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

Patch lbvs_consent 2.1.1-2.2.0 to support openbabel-3 #24677

Merged
merged 13 commits into from
Apr 10, 2024

Conversation

jmid
Copy link
Contributor

@jmid jmid commented Oct 26, 2023

The previous PR #24545 patched conf-openbabel to install openbabel-2 or 3.

The lbvs_consent however still hardcodes an openbabel-2 include path which causes CI build errors, and thus adds noise:

lbvs_consent.2.1.1 (failed: The compilation of lbvs_consent.2.1.1 failed at "make obmaccs".)
lbvs_consent.2.1.2 (failed: The compilation of lbvs_consent.2.1.2 failed at "make obmaccs".)
lbvs_consent.2.1.3 (failed: The compilation of lbvs_consent.2.1.3 failed at "make obmaccs".)
lbvs_consent.2.2.0 (failed: The compilation of lbvs_consent.2.2.0 failed at "make obmaccs".)

This follow-up PR thus patches the Makefiles for lbvs_consent 2.1.1-2.2.0 to utilize #24545.

I've filed an upstream PR with the corresponding change here: UnixJunkie/consent#15

@jmid
Copy link
Contributor Author

jmid commented Oct 26, 2023

I somehow managed to include illegal white chars again... 🤦

I've therefore checked that all 4 four opam files lint locally.
lbvs_consent.2.1.1/opam and lbvs_consent.2.1.2/opam trigger a lint warning locally (and on the CI):

  warning 62: License doesn't adhere to the SPDX standard, see https://spdx.org/licenses/: "GPL"

This is unrelated to the changes in this PR though (which doesn't touch the license field)

@jmid
Copy link
Contributor Author

jmid commented Oct 26, 2023

I see FreeBSD failures on conf-python-3.9.0.0

#=== ERROR while compiling conf-python-3.9.0.0 ================================#
"python3": command not found.

which should be solved by #24652

@jmid
Copy link
Contributor Author

jmid commented Oct 26, 2023

9c86a07 adds a lower bound on cpm which seems to have restructured to have a Cpm module with the 4.0.0 release.

@jmid
Copy link
Contributor Author

jmid commented Oct 26, 2023

  • 92fdc8e adds a lower bound for minivpt (only a dep for 2.1.1 and 2.1.2)
  • 2cf1ce6 adds a lower bound for batteries which added BatString.split_on_string in 2.11.0

@jmid
Copy link
Contributor Author

jmid commented Oct 26, 2023

@jmid
Copy link
Contributor Author

jmid commented Oct 26, 2023

Overall there remains

I suggest to wait until #24652 #24679 and #24680 have been merged, and then rerun CI before merging this one.
I would expect for only conf-rdkit CI failures to remain at that point.

@jmid
Copy link
Contributor Author

jmid commented Oct 26, 2023

Rebased on master to trigger a new run, since the above 3 PRs have now been merged.
I expect conf-rdkit CI failures.

@jmid
Copy link
Contributor Author

jmid commented Oct 27, 2023

There are still minivpt CI errors unfortunately. #24696 bumps its lower obuild bound to address it.

@jmid
Copy link
Contributor Author

jmid commented Oct 27, 2023

There are also still extunix CI failures, which #24698 should address.

@jmid
Copy link
Contributor Author

jmid commented Oct 27, 2023

Rebased on master again after #24696 and #24698 have been merged 🍿 🤞

@jmid
Copy link
Contributor Author

jmid commented Oct 27, 2023

CI revealed a missing bound on camlzip:

#=== ERROR while compiling lbvs_consent.2.1.1 =================================#
# context              2.2.0~alpha3~dev | linux/x86_64 | ocaml-base-compiler.4.05.0 | pinned(https://github.com/UnixJunkie/consent/archive/v2.1.1.tar.gz)
# path                 ~/.opam/4.05/.opam-switch/build/lbvs_consent.2.1.1
# command              ~/.opam/opam-init/hooks/sandbox.sh build dune build -p lbvs_consent -j 31
# exit-code            1
# env-file             ~/.opam/log/lbvs_consent-6-45bab3.env
# output-file          ~/.opam/log/lbvs_consent-6-45bab3.out
### output ###
#       ocamlc src/.lbvs_consent.objs/byte/lbvs_consent__MyUtils.{cmi,cmo,cmt} (exit 2)
# (cd _build/default && /home/opam/.opam/4.05/bin/ocamlc.opt -w -40 -g -bin-annot -I src/.lbvs_consent.objs/byte -I /home/opam/.opam/4.05/lib/batteries -I /home/opam/.opam/4.05/lib/bitv -I /home/opam/.opam/4.05/lib/bytes -I /home/opam/.opam/4.05/lib/cpm -I /home/opam/.opam/4.05/lib/dolog -I /home/opam/.opam/4.05/lib/minivpt -I /home/opam/.opam/4.05/lib/num -I /home/opam/.opam/4.05/lib/ocaml/threads -I /home/opam/.opam/4.05/lib/parmap -I /home/opam/.opam/4.05/lib/zip -no-alias-deps -open Lbvs_consent -o src/.lbvs_consent.objs/byte/lbvs_consent__MyUtils.cmo -c -impl src/myUtils.ml)
# File "src/myUtils.ml", line 74, characters 2-23:
# Error: Unbound value Gzip.output_substring

Gzip.output_substring was added in camlzip.1.06 so be1abc2 adds that as a lower bound... 🍿

@jmid
Copy link
Contributor Author

jmid commented Oct 27, 2023

  • 34f8b6a adds a lower bound to make BatString.chop available
  • 9281158 adds a lower bound for parmap on the first non-rc package after 0.9.1 which is incompatible API-wise

@jmid
Copy link
Contributor Author

jmid commented Oct 27, 2023

The remaining failures are all from conf-rdkit. #24708 is an attempt at adding FreeBSD support for it.

@mseri
Copy link
Member

mseri commented Oct 30, 2023

In general I am against patches on released packages and ask to release a -1 patch version. In this case I would be in favour since it is a strict improvement over what we have and reduces drastically the failures. We need to discuss this at the next maintainer meeting and see if we all agree before we can merge

@mseri
Copy link
Member

mseri commented Apr 10, 2024

Since this was merged upstream and it is a specialized software that would become available on a larger set of platforms, I am going to accept and merge the change

@mseri mseri merged commit 840d322 into ocaml:master Apr 10, 2024
1 check failed
@mseri
Copy link
Member

mseri commented Apr 10, 2024

Thanks and sorry for the delay

@jmid jmid deleted the patch-lbvs_consent branch April 10, 2024 13:19
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.

None yet

2 participants