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

Add loading of packaged libraries by feature flag #51

Merged
merged 2 commits into from
Dec 30, 2019

Conversation

Xcodo
Copy link
Contributor

@Xcodo Xcodo commented Dec 27, 2019

From #45:

I refactored the configuration loading to make it easy to add loading of installed configuration: See: https://github.com/kraigher/rust_hdl/blob/master/vhdl_parser/src/config.rs#L236-L239
You only need to add a load_installed_config there to implement it.

I've implemented this function. I'm only up to chapter 5 in the Rust book, so improvements are welcome!

I've not added any tests. Am I right in thinking there's none for the other configuration loading functions either? I'm also couldn't work out how you add tests for whether a feature flag is enabled or not anyway.

I think we should use the cfg! macro or attribute to set the relative path to the installed vhdl_ls.toml file. By default it think the relative path should assume the binary ends up in target/release such that when building locally with cargo build --release during development it will just work without having to create the installed folder structure.

When building for release we can set a --cfg clag to rustc (https://doc.rust-lang.org/rustc/command-line-arguments.html#--cfg-configure-the-compilation-environment) such that the relative path is according to the install folder format.

I was a bit less successful here. From reading up on this the supported way of doing this with Cargo seems to be through feature flags. I implemented that for the vhdl_parser project with the packaged feature, which is also added to the vhdl_ls project to pass it through.

While this seems to be working for building it locally to the project

rust_hdl\vhdl_parser> cargo build --features "packaged"

It looks like feature flags aren't supported when building at the workspace level at the moment. The CI build could build from within each folder separately, but that's not what it does now.

@kraigher
Copy link
Member

Looks good. Building releases from the workspace might not be necessary. The release can be built from the crate instead.

@Xcodo
Copy link
Contributor Author

Xcodo commented Dec 28, 2019

Looks like the way to do this with the cargo action is this. I'll implement that once @Bochlin has finished improving the CI setup.

@Xcodo Xcodo changed the title WIP: Add loading of packaged libraries by feature flag Add loading of packaged libraries by feature flag Dec 29, 2019
@Xcodo
Copy link
Contributor Author

Xcodo commented Dec 29, 2019

Ready to go now I think @kraigher

@kraigher kraigher merged commit b40b2c7 into VHDL-LS:master Dec 30, 2019
@Xcodo Xcodo deleted the installed-libs branch January 17, 2020 21:10
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.

2 participants