Should support multiple LLVM-sys versions about inkwell HOT 18 CLOSED

Worth noting that a particular version will only be considered supported if all functions intended to be supported are supported. That is, there are some functions that may be deemed too redundant to be useful.

For example "is_int()" is useful in LLVM because of their hyper abstraction, but in our IntType/IntValue it would never be useful and will be skipped because that piece of information is ingrained into the type system.

Or, if a function is deprecated from v3.6 onward (and likely has a replacement function), it may very well be skipped as well.

That said, nearly all LLVM functions should be supported, except in similarly outlying cases.

from inkwell.

Cargo apparently does not support dependencies to be specified depending on the current feature set (see rust-lang/cargo#4576). An alternative would be to force the user to:

1. Override inkwell's default llvm_sys version using the [replace] section.
2. Specify the feature associated with this version using the [dependencies.inkwell] section and the features field.

Needless to say, this solution is very inconvenient, as it requires the user to make multiple changes, and may lead to conflicts.

from inkwell.

1. is generally only used when a dependency's dependency is broken so it would be pretty nonstandard to use that.
2. I think fits the bill much better as this is standard procedure for always opting into a library's feature(s). The only difference for us, I think, is that it will be a hard requirement to have one and only one of the llvm version features.

from inkwell.

I tried my hand at 2) this past week and it doesn't seem possible with cargo as it stands today. I thought custom git repo dependencies could work, but it didn't. Maybe 1) could work, but this seems super unideal.

I'm wondering if maybe we should propose an RFC or just wait for multiple dependencies to be released at some point. If this issue ends up being the only thing holding us back from releasing a 0.1.0, then we should go ahead with 1) so that we can get it out there. But I think for now, it's more ideal to not go forward with 1) and hope a better solution comes out in the mean time.

from inkwell.

Oh, I actually meant using both at the same time, I don't think one can work without the other.

from inkwell.

That might be a viable solution (short of a real cargo supported solution) for distribution, but does seem really painful to get working with travis. Might need to string insert/replace text into the cargo.toml file, which can be a really fragile hack..

from inkwell.

I'd be willing to look at adding support for 5.0, though I'm not sure where to begin. Perhaps it makes sense to write a guide to adding new versions?

from inkwell.

That would be great, thanks! Unfortunately at the moment we only support a single version. I'm currently working on getting 3.6 support in so that we do have a baseline for multiple versions. So maybe when that's done I can show you the relevant changes? I will also try and document it as you suggested (an excellent idea) but that may or may not be available right away. Will add that to the above checklist.

from inkwell.

@6a So apparently you can't specify a dependency's version via replace or patch. You can only point it to a git repo or local path. This just keeps getting worse lol. I might have to have users point at the GH git mirror of llvm-sys' bitbucket hg repo since hg isn't supported by cargo 👀

from inkwell.

@TheDan64 How about using [patch.crates-io] section to specify the branch to use?
Looks like Peter Marheine maintains a branch for each version.

from inkwell.

Yeah, that's what I was referring to (can't use his original bitbucket repo though, only his github mirror: https://github.com/tari/llvm-sys.rs)

from inkwell.

Ah, indeed. Did you try going through the GH mirror then?

from inkwell.

Trying, but it doesn't seem to work.

I did:

[patch.crates-io]
llvm-sys = { git = "https://github.com/tari/llvm-sys.rs", branch = "llvm-3.6" }

but it still built 37.0.5 for some reason even after a fresh build. If I look through my Cargo.lock I see this:

[[patch.unused]]
name = "llvm-sys"
version = "36.0.6"

Not sure why it's unused, though lol

from inkwell.

@blanham I don't have an official guide yet; but this PR should give you a rough idea of what needs to be done if you're still interested: #31

It boils down to:

• Add an llvm-X build to the .travis.yml
• Update the README.md to specify the correct llvm-x branch
• Update Cargo.toml to support a new feature flag
• Update src/lib.rs to support a new feature flag
• Add #[cfg()] attributes to make new functions only from the version they were added onward, and to version no longer supported functions
• Add tests
• Add #[cfg()] attributes to tests if needed
• I have to create a branch for your llvm version, once the above steps are completed

from inkwell.

Forgot to put up a PR to link to this issue; but LLVM 5.0 support is now available

from inkwell.

LLVM 6.0 support has recently landed

from inkwell.

Gah, LLVM 7 has been released. Going to add initial support for it soon.

from inkwell.

We now support multiple LLVM versions in a single branch/crate. Closing.

from inkwell.