Skip to content
This repository has been archived by the owner on Mar 8, 2020. It is now read-only.

Outdated drivers are treated as up-to-date on SDK updates #235

Closed
dennwc opened this issue Feb 20, 2018 · 11 comments
Closed

Outdated drivers are treated as up-to-date on SDK updates #235

dennwc opened this issue Feb 20, 2018 · 11 comments
Assignees
Labels

Comments

@dennwc
Copy link
Member

dennwc commented Feb 20, 2018

Currently drivers manifests has no mentions of version of SDK they were written for. It leads to a situation when driver might become outdated and incompatible, while still being listed as beta or even stable.

I propose to include new (required) field to manifest with semantic version of SDK. This field will be overwritten by bblfsh-sdk update and will be considered when interpreting driver status.

As always, minor version difference will not affect driver status, but major difference will automatically drop the status to inactive.

@juanjux
Copy link
Contributor

juanjux commented Feb 21, 2018

We were planning to add a dependency to the SDK version using glide, but this could also be additionally done.

@dennwc
Copy link
Member Author

dennwc commented Feb 21, 2018

Glide (why not dep?) will fix an issue from driver's perspective (SDK changes), but will not directly solve issue from bblfshd perspective (drivers decomes outdated).

Potentially we could read deps file and check what version is mentioned there - it will be the version of SDK drivers supports, and bblfshd can filter these values depending on SDK version it was compiled for.

@dennwc
Copy link
Member Author

dennwc commented Feb 22, 2018

OK, I will require all drivers to add dep config with SDK and will use it's version to decide if drivers is usable by bblfshd or not. It will require bblfshd to contain a version of SDK it was compiled for, thus either build scripts should be updated, or SDK should maintain a constant with major version number at least.

@juanjux
Copy link
Contributor

juanjux commented Mar 15, 2018

Outside of the convenience of using dep, I think the SDK dependency should be stated in the manifest just like we do with the required Go version because that's the purpose of the driver's manifest.

@dennwc
Copy link
Member Author

dennwc commented Mar 15, 2018

Is it just for quick access? Driver discovery can pull dep file as it pulls MAINTAINERS currently.

@juanjux
Copy link
Contributor

juanjux commented Mar 15, 2018

It's because the manifest should have all the important versions needed in a single place, I think (currently it has the runtime version of the language and the Go versions). Then the bblfsh-sdk update command could update the Gopkg.toml file from the manifest as it does with other things.

@dennwc
Copy link
Member Author

dennwc commented Apr 25, 2018

I agree that manifest should contain this information as well. I propose to parse both MAINTAINERS and Gopkg.toml as a part of bblfsh-sdk update and update manifest accordingly. This will allow dropping the parsing code from driver discovery and will remove some unnecessary calls to Github API.

@dennwc
Copy link
Member Author

dennwc commented Aug 1, 2018

The first step is done - new drivers will include an SDK version in the Gopkg.toml.

@bzz
Copy link
Contributor

bzz commented Nov 22, 2018

🎉 What would be the next action items here?

@dennwc
Copy link
Member Author

dennwc commented Nov 22, 2018

First of all, the bblfsh-sdk tool will need to detect vendor directory and run the bblfsh-sdk from the vendored SDK. This way we can make sure that bblfsh-sdk is the same version as the SDK package listed in the manifest. But this can be solved separately (see #325).

We might be able to solve it by updating bblfshd to use the major SDK version it was compiled for (cannot infer minor directly), and skip drivers with different major versions when installing.

@juanjux
Copy link
Contributor

juanjux commented Feb 12, 2019

Is this still happening? driver install --all doesnt try to install the Rust or Ocaml drivers for me right now. Temptatively closing, please reopen if this is not the case.

@juanjux juanjux closed this as completed Feb 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants