System externals to specify extra paths (C_INCLUDE_PATH, LIBRARY_PATH, PATH)

[Fabien-Chouteau]

In the context of using sldada with SDL2 from msys2 on Windows I got into a problem that is not very easy to describe, it deals with include paths and sdl2-config.

At the moment, sdlada has three C source files that include headers from SDL2. The includes look like this: #include <SDL.h>. The path to the include is supposed to be provided by sdl2-config --cflags which returns options like -I/usr/include/SDL2.

With msys2 the sdl2-config return a UNIX path relative to the "context"/"world" of msys2: /mingw64/include/SDL2. This is hardcoded and doesn't depend at all on where msys2 is installed.

The problem is that a compiler not running in the msys2 context will not be able to understand this path, so the includes cannot be resolved.

One potential solution I see for this problem is to allow external crate to specify include paths depending on the distribution. For instance:

[[external]]
kind = "system"
    [external.origin.'case(distribution)']
    'msys2' = ["mingw-w64-x86_64-SDL2"]
    [external.c_include_path.'case(distribution)']
    'msys2' = ["$MSYS_ROOT/mingw64/include/SDL2"]

Alire would then add this path to the C_INCLUDE_PATH environment variable before build a project.

(The $MSYS_ROOT keyword can be optional and all paths considered as relative to the root dir of the distribution)

@mosteo, @pmderodat what do you think of this?

[mosteo]

Two points you can confirm for me, to see if I'm not confusing something:

• Do I understand correctly that sdl2-config is not really called with this solution? Could that bite us in the future with some other library that requires a -config script? My gut feeling is not a problem, but I'm not sure.
• IIUC, $MSYS_ROOT will be replaced by alr, which detects where it is installed?

With these caveats in my understanding:

Would we be more future-proof by having something general that sets/appends/prepends to environment variables? (which reminds me: there is an undocumented property that prepends items to PATH (#178). IIRC it's broken after the index migration. Perhaps we could build on that?)

If you see that's overkill, I trust your better understanding of the issue.

Either way, loading dynamic properties is a somewhat convoluted part right now, so I could tackle it if you prefer and simplify the internals in the process (#323).

[Fabien-Chouteau]

* Do I understand correctly that `sdl2-config` is not really called with this solution? Could that bite us in the future with some other library that requires a `-config` script? My gut feeling is not a problem, but I'm not sure.

sdl2-config will still be called by the makefiles of sdlada. This can be a problem in the future, to be honest I don't have enough data points at the moment.

* IIUC, `$MSYS_ROOT` will be replaced by `alr`, which detects where it is installed?

Yes. It could be $DISTRO_ROOT or whatever else BTW. For UNIX, distro the root will be /. As I said we can also consider that all the paths are relative to the distro root and not have a keyword at all.

With these caveats in my understanding:

Would we be more future-proof by having something general that sets/appends/prepends to environment variables? (which reminds me: there is an undocumented property that prepends items to PATH (#178). IIRC it's broken after the index migration. Perhaps we could build on that?)

You mean in other types of crates as well?

[mosteo]

Thanks for the clarifications.

You mean in other types of crates as well?

I mean, instead of directly having a property named c_include_path, having an env-export or similar that allows setting any env var, a la gpr-set-externals. In that case I guess it would make sense that it worked generally, yes.

[Fabien-Chouteau]

ok so something like:

    [environment.c_include_path.append]
    ["$MSYS_ROOT/mingw64/include/SDL2"]

    [environment.whatever.prepend.'case(distribution)']
    'ubuntu' = ["PLOP"]

or

[environment-append]
C_INCLUDE_PATH = "release"

[environment-prepend.'case(os)']
linux   = { PATH = "/plip/plop" }
windows = { PATH = "C:\test" }

[mosteo]

Exactly. Since the current loading infrastructure relies in prop=value or prop.case().enum = value expressions (#186), your second proposal works as is. Though, I like your first example encapsulating everything under a single environment property. The resulting combined syntax would be:

[environment.append]
C_INCLUDE_PATH = "release"

[environment.'case(os)'] # inlined
linux   = { prepend.PATH = "/plip/plop" }
windows = { prepend.PATH = "C:\test" }

[environment.'case(os)'.macos.set] # not inlined
OS = "MacOS"
ALIRE = "true"

[mosteo]

@Fabien-Chouteau , although you said you'd tackle this one I was thinking that I could do the loading/saving part, and you can take from there? Just because the conditional expressions infrastructure is a bit convoluted right now.

[Fabien-Chouteau]

ok thank you, I can find other things to work on in the meantime.

[Fabien-Chouteau]

@mosteo I was starting to think about the back-end of this. And I wonder if we should allow a set action.

As set would potentially conflict with other environment action on the same variable:

What would be the result of two crates setting conflicting values on a variable:

[environment.set]
PLOP = "something"

[environment.set]
PLOP = "something else"

We can of course detect this and trigger an error. But I wonder if we should avoid set entirely.

[mosteo]

I see... I fear that if we remove it entirely, the need will arise. I'd say to err if trying to set a variable that's already set.

I've looked around in opam and cargo but I didn't find anything conclusive.

[mosteo]

Implemented by #389, #452