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

Build with dylib #7

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

Conversation

vthemelis
Copy link

@vthemelis vthemelis commented Aug 3, 2023

First crack at fixing #6

See my comment here for more details: #6 (comment)

Before this patch, one could successfully do:

dune exec ./bin/pyre_parse.exe

but

dune exec ./bin/pyre_parse.bc

would fail.

Now, both work!

@vthemelis vthemelis marked this pull request as draft August 3, 2023 23:15
@@ -20,18 +20,17 @@
11.0
(progn
(run ./configure)
(run make libpython3.11.a))))
(copy vendor/libpython3.11.a libpython.a)
Copy link
Author

Choose a reason for hiding this comment

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

I think that it's likely easier to just create a static library here and statically link it to both the static and the dynamic binding of taglessFinal (ie the artefact of binding.c)

(foreign_stubs
(language c)
(names binding)
(flags :standard -Ivendor/Include -I.))
(c_library_flags -lpython -lutil -lpthread)
(foreign_archives python))
(c_library_flags -lutil -lpthread -lpython -L%{project_root}/_build/default/lib/taglessFinal/))
Copy link
Author

Choose a reason for hiding this comment

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

This is hacky! I should be able to get the outputs of the rule in the context of the C build without adding the -L command.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah this creates hard dependency on where the build directory is. Any chance we could get the output without hardcoding the path?

Copy link
Author

Choose a reason for hiding this comment

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

That's what I'm looking for but don't know of a way at the moment 😢. Hopefully there's a way and someone in ocaml/dune#8335 can show me.

@vthemelis vthemelis marked this pull request as ready for review August 4, 2023 11:54
@vthemelis vthemelis marked this pull request as draft August 4, 2023 11:58
@vthemelis vthemelis marked this pull request as ready for review August 4, 2023 12:00
@vthemelis vthemelis force-pushed the dynamic-library branch 3 times, most recently from fb52098 to 1e8a5d9 Compare August 4, 2023 14:26
@grievejia
Copy link
Owner

LGTM except for the pathing in the build. Would also be nice if this change can be recorded on top of the CHANGES.md file.

Before this pathch, one could successfully do:

```
dune exec ./bin/pyre_parse.exe
```

but
```
dune exec ./bin/pyre_parse.bc
```
would fail.

Now, both work.

Signed-off-by: Vasilis Themelis <vdthemelis@gmail.com>
@vthemelis
Copy link
Author

vthemelis commented Aug 4, 2023

LGTM except for the pathing in the build. Would also be nice if this change can be recorded on top of the CHANGES.md file.

Thanks! I added a new entry to the CHANGES.md file. I will consult with the dune team about the dependency to the path. ~~I think that it's harmless albeit hacky and ~~I would also like to replace it with something that was designed for this use case.

I smoke tested this by adding my changes to an opam pin and running pyre's main executable and it seems to be working fine. The build is also green.

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