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

allow non-opam installations #52

Merged
merged 3 commits into from
May 16, 2019
Merged

allow non-opam installations #52

merged 3 commits into from
May 16, 2019

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented May 13, 2019

Makefile: provide rules for flags/libs.tmp and flags/cflags.tmp, clean: rm them
.gitignore: ignore remporary generated flags files

this improves local non-opam builds after c5bf86c /cc @TheLortex

Makefile: provide rules for flags/libs.tmp and flags/cflags.tmp, clean: rm them
.gitignore: ignore remporary generated flags files
pkg-config can take multiple arguments and will combine all
dependencies, so no need for any loops and so many intermediate steps.
@mato
Copy link
Contributor

mato commented May 15, 2019

@hannesm I pushed 7904eee to your branch which simplifies things a bit, AFAICT the outputs are identical. FWIW I'm not too happy about the use of the

echo "("`cat ...`")"

construct, since if there are any shell constructs in the output of cat then they will get evaluated by the shell, but if you're not concerned about that then I'm happy to merge this.

@hannesm
Copy link
Member Author

hannesm commented May 16, 2019

@mato thanks! I did neither write nor merge the echo "("`cat ..`")" lines, and agree with you that it is a bad idea. I suspect they can be rewritten as:

echo "(" > flags/libs
env PKG_CONFIG_PATH="$(shell opam config var prefix)/lib/pkgconfig" \
	    pkg-config $(PKG_CONFIG_DEPS) --libs  | sed -e 's!@@PKG_CONFIG_EXTRA_LIBS@@!$(PKG_CONFIG_EXTRA_LIBS)!' >> flags/libs
echo ")" >> flags/libs

(but likely there are some superfluous or missing line endings etc.)

my initial motivation for this PR that before it with a cloned ocaml-freestanding running make errored, and since I wanted to improve ocaml-freestanding (see the other PRs), this was my minimal attempt to fix it.

I guess, esp. with respect to mirage/mirage#969 (comment) I'm still lacking the overall design of mirage 4, in respect to usage of dune and pkg-config.

@TheLortex
Copy link
Member

For the echo "("`cat ..`")" ugliness, that's my bad. That PR exposing flags using files was probably too early..

Regarding dune and pkg-config:

  • ocaml-freestanding is currently not a dune project, but it's at the frontier between dune packages (like mirage-solo5) and non-dune packages (solo5-bindings-...).
  • in order to gather compilation settings (cflags, link flags, ..) there are two ways:
    • Using pkg-config: this works well in most of the cases, but is difficult in an opam-less environment as there won't be an obvious pkg-config path.
    • Using files that dune can include: this is what Expose flags though files and enable the use of -runtime-variant #50 enabled and it allows to get rid of pkg-config in the mirage build system. It's mainly a hack that precomputes the flags. It should work better in an opam-less environment as these flags files are accessible directly from the dune workspace (using %{lib:ocaml-freestanding:cflags}% to refer to the cflags for example).

It looks like we're moving toward a Mirage 4 that depends less and less on opam. According to what's suggested in mirage/mirage#969 (comment), most of the sources need to be locally downloaded instead of being installed with opam.

@hannesm
Copy link
Member Author

hannesm commented May 16, 2019

IMHO: let's merge this now since it improves development of ocaml-freestanding, and adapt ocaml-freestanding piecewise with mirage4 development if needed. at least if I unerstand @TheLortex correctly, having both pkg-config and libs/cflags helps with the mirage4 development atm.

@mato
Copy link
Contributor

mato commented May 16, 2019

@hannesm @TheLortex Does dune interpret \n in the included files in any way? If not, then I have a solution for replacing the echo and cat with a single sed invocation.

@mato
Copy link
Contributor

mato commented May 16, 2019

In other words, is

(
foo
bar
baz
)

equivalent to

(foo bar baz)

?

@hannesm
Copy link
Member Author

hannesm commented May 16, 2019

@mato I don't know. from my experience with dune files, they don't care about whitespace, i.e. your above example is identical

@mato
Copy link
Contributor

mato commented May 16, 2019

@hannesm @TheLortex I checked with the dune metadata format doc (https://dune.readthedocs.io/en/latest/project-layout-specification.html#metadata-format) and it does seem that extra whitespace / newlines are not significant in a list, so I've pushed 0bb2f6f to remove the problematic use of cat.

.gitignore Outdated Show resolved Hide resolved
According to the dune metadata format [1], extra whitespace and newlines
are not significant in a list, so remove the fragile use of "cat" from
flags/ generation, replacing it with a single invocation of "sed".

[1] https://dune.readthedocs.io/en/latest/project-layout-specification.html#metadata-format
@hannesm
Copy link
Member Author

hannesm commented May 16, 2019

i approve this change as it is now.

@mato mato merged commit 10c6342 into mirage:master May 16, 2019
@hannesm hannesm deleted the no-opam branch May 16, 2019 13:52
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

3 participants