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

[new release] allegro5 (0.1) #27169

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Frigory33
Copy link

OCaml Allegro 5

CHANGES:

First release. Includes elements of the following Allegro 5 modules:

  • Displays
  • Events
  • Graphics
  • Keyboard
  • Mouse
  • System
  • Time
  • Timer
  • Font addons
  • Image addon
  • Primitives addon

@Frigory33
Copy link
Author

I don’t know how to add an external dependency to my library. It needs the Allegro 5 development library and its addons. Thus the build failed. What should I do?

@shonfeder
Copy link
Contributor

First, thank you for taking the time to publish your work on these bindings!

I don’t know how to add an external dependency to my library. It needs the Allegro 5 development library and its addons. Thus the build failed. What should I do?

opam is able to records system dependencies for packages available in a number of popular distros via the depext field. As noted in those docs, the depext field should be used in a conf- package, that declares a (reusable) system dependency. Here's an example of the conf- package for git: https://github.com/ocaml/opam-repository/blob/master/packages/conf-git/conf-git.1.1/opam

For systems where the dependency is not available from a supported package manager, you can add a post-install message that is printed only on failure, pointing to users towards how the dependency can be met. Here's an example:

https://github.com/toots/opam-repository/blob/d31b2b983a4cae4ba367ae61c522f8570ff6783a/packages/ffmpeg-avdevice/ffmpeg-avdevice.1.2.1/opam#L36-L41

For these cases, we an also add the x-ci-accept-failure field to help the CI know that failures on the given platforms is expected.

Let us know if you have any additional questions, and/or don't hesitate to ask on https://discuss.ocaml.org/

@Frigory33
Copy link
Author

Thank you very much for your support!

I don’t know how to deal with MSYS2. There is no Allegro package in Cygwin but there is one in MSYS2.

And I don’t know how to deal with windows with x-ci-accept-failure.

Copy link
Contributor

@shonfeder shonfeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the updates! Things are coming along well here :)

I have suggested one fix for the tests failures here, and one suggestion to extend some windows packaging support.

Could you also take a look some of the build failures to see if they can be fixed or if they are expected?

packages/conf-allegro5/conf-allegro5.1/opam Show resolved Hide resolved
packages/allegro5/allegro5.0.1/opam Outdated Show resolved Hide resolved
@Frigory33 Frigory33 force-pushed the release-allegro5-0.1 branch 3 times, most recently from 4635822 to 8cc93b8 Compare January 3, 2025 10:20
@Frigory33
Copy link
Author

I’ve done what I could, but now there are problems with pkg-config. My packages just need the pkg-config command. I saw in the conf-sdl2 opam file that there is a weird case for Cygwin, using pkgconf instead of pkg-config. Should I simply use pkgconf instead of pkg-config everywhere?

I also changed my test and my release of the ocaml-allegro-5 repository, but I don’t know how to make the tests here to use the new files, while it’s still the release “0.1.”

@shonfeder
Copy link
Contributor

Hello! Thank you again for your careful work here! I apologize for the delay: we are still getting caught up again after the holiday.

there are problems with pkg-config

Ah yes, I see. It looks like this is a problem with mingw-w64-shims:

  The following actions will be performed:
  === recompile 1 package
    - recompile mingw-w64-shims 0.2.0  [uses conf-pkg-config]

This needs to be fixed in that conf package, iiuc, so out of scope here.

I also changed my test and my release of the ocaml-allegro-5 repository, but I don’t know how to make the tests here to use the new files, while it’s still the release “0.1.”

The cleanest way would be to update this PR to point to the corrected, updated version of your package. With this PR, you can just update the version number in the directory name, and update the sources and sha. If you are using opam-publish or dune-release you can run them in a mode to produce the new opam file, and replace the content here with that. Or you may open a new PR with your updated release and close this one.

This is advisable, because it will ensure that when we check your package during reversed dependency checks, we do not get false positives. Otherwise, you will not protected by our CI against breaking changes of your dependencies. Please let me know if you have questions about how to do this! I am happy to support.

CHANGES:

First release. Includes elements of the following Allegro 5 modules:

- Displays
- Events
- Graphics
- Keyboard
- Mouse
- System
- Time
- Timer
- Font addons
- Image addon
- Primitives addon
@Frigory33 Frigory33 force-pushed the release-allegro5-0.1 branch from 3e4c1fb to da45543 Compare January 9, 2025 16:43
@Frigory33
Copy link
Author

I think I’ve done what you said, but now there’s a new failing check (maintenance-intent present)… I don’t understand.

@shonfeder
Copy link
Contributor

shonfeder commented Jan 9, 2025

That's a newly added check and it's not stable yet (perhaps should be removed from CI for the moment, see #27234). Sorry for the noise there!

Unfortunately it seems all the builds are failing now tho.

@Frigory33
Copy link
Author

The build fails with OCaml 4 variants. I get the same problem locally with an opam switch. I don’t know what to do… Do you have an idea of where this bug comes from? (The errors are not specific to the display and timer modules of my library; I get the same errors if I keep only two modules.) Should I require OCaml 5 for my package? It might be the only solution…

Also, can you help me on publishing the documentation of my package on GitHub with dune-release? I don’t understand what I should do.

@shonfeder
Copy link
Contributor

I will take a look at both the builds on OCaml 4 and your documentation situation this weekend! I am optimistic that I'll be able to help on both accounts. Thanks again for your persistence and patience here!

@shonfeder shonfeder self-requested a review January 12, 2025 04:31
@shonfeder
Copy link
Contributor

The build fails with OCaml 4 variants.

I wanted to note quickly that the build failures here are not limited to OCaml 4.x. See, e.g., https://opam.ci.ocaml.org/github/ocaml/opam-repository/commit/da45543bcf6ada03ab09d168b588f94a1c22a7cb/variant/distributions,fedora-41-ocaml-5.3,allegro5.0.2

[ERROR] The compilation of allegro5.0.2 failed at "dune build -p allegro5 -j 71 --promote-install-files=false @install".

#=== ERROR while compiling allegro5.0.2 =======================================#
# context              2.4.0~alpha1~dev | linux/x86_64 | ocaml-base-compiler.5.3.0 | pinned(https://github.com/Frigory33/ocaml-allegro-5/releases/download/0.2/allegro5-0.2.tbz)
# path                 ~/.opam/5.3/.opam-switch/build/allegro5.0.2
# command              ~/.opam/opam-init/hooks/sandbox.sh build dune build -p allegro5 -j 71 --promote-install-files=false @install
# exit-code            1
# env-file             ~/.opam/log/allegro5-7-8d9a0a.env
# output-file          ~/.opam/log/allegro5-7-8d9a0a.out
### output ###
# File "lib/dune", lines 19-26, characters 0-277:
# 19 | (library
# 20 |  (name al5)
# 21 |  (public_name allegro5)
# 22 |  (foreign_stubs
# 23 |   (language c)
# 24 |   (names al5 display events graphics keyboard mouse system time timer font image primitives)
# 25 |   (flags -std=c11 -Wall -O2 -I -fPIC (:include c_flags.sexp)))
# 26 |  (c_library_flags (:include lib_flags.sexp)))
# (cd _build/default && /home/opam/.opam/5.3/bin/ocamlmklib -g -o lib/al5_stubs lib/timer.o lib/time.o lib/system.o lib/primitives.o lib/mouse.o lib/keyboard.o lib/image.o lib/graphics.o lib/font.o lib/events.o lib/display.o lib/al5.o -ldopt -lallegro_image -ldopt -lallegro_primitives -ldopt -lallegro_ttf -ldopt -lallegro_font -ldopt -lallegro)
# /usr/bin/ld: lib/timer.o: relocation R_X86_64_32 against `.rodata.str1.1' can not be used when making a shared object; recompile with -fPIC
# /usr/bin/ld: failed to set dynamic section sizes: bad value
# collect2: error: ld returned 1 exit status

@shonfeder
Copy link
Contributor

Please note that I am not at all expert at working with C-stubs!

I have been trying to dig into this error. In case it is useful, these are some of the resources and discussions I've consulted:

However, I wonder if the solution may lie in ocaml/dune#5809 . The latter issue includes a proposed workaround in the issue description, and tho it was supposedly fixed in an earlier version of dune, a possible regression is reported there.

In the switch where you are able to reproduce this, could show what you see in the build logs, as in ocaml/dune#5809 (comment) ? So, could you run something like

$ dune --version
$ dune build
$ grep '/usr/bin/gcc' _build/log

and share the results? Perhaps also just investigating the _build/log on your own to see if you the compilation of the c stubs looks like it may answer to the error

# /usr/bin/ld: lib/timer.o: relocation R_X86_64_32 against `.rodata.str1.1' can not be used when making a shared object; recompile with -fPIC

If this does not help point the way, you may want to consider seeking help on https://discuss.ocaml.org/ ! Many helpful folks with deep expertise interfacing with c there :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants