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

src: add deps to process.versions #166

Closed
wants to merge 7 commits into from

Conversation

RafaelGSS
Copy link
Member

@RafaelGSS RafaelGSS commented Aug 20, 2024

Fixes: #163

New dependency list:

  • asserts-cpp - I didn't think is a valid addition to process.versions as it doesn't really seem a dependency but a helper in the wrong folder.
  • curl
  • gprc - This is a third-party dependency - We use it through opentelemetry. The only reason it's top level dependency was to fix a windows linker issue
  • nlohmann
  • ncm-ng - It doesn't seem like a nsolid runtime dependency, but an alternate binary if I'm reading N|Solid dependencies versions are not listed in process.versions nor in nsolid.info() #163 correctly. Therefore, it doesn't seem to apply to process.versions.
  • nsolid-cli - Already covered
  • nsuv - This library doesn't hold VERSION information, so at the moment I decided to not include it and work on this in that repo.
  • opentelemetry-cpp
  • protobuf - Same as grpc.
  • sodium - Same as grpc
  • zmq

Note we are extracting the version directly in the deps/ folder. The Node.js sometimes create a src/X_version. and update it through tools/updater.sh. If you prefer I can do that, but it will require creating an updater for all of our dependency (I was planning to do it in a second PR).

@santigimeno
Copy link
Member

Code looks good to me though I'd like we get to a decision on whether add this to process.versions directly or create a specific property process.nsolidVersions as I mentioned in #163 .

@RafaelGSS
Copy link
Member Author

Code looks good to me though I'd like we get to a decision on whether add this to process.versions directly or create a specific property process.nsolidVersions as I mentioned in #163 .

It seems to add a process.nsolidDeps.* a good choice. I can change the PR. @edsadr @trevnorris any thoughts?

@RafaelGSS
Copy link
Member Author

While working on it, I've been thinking @santigimeno... I think believe that adding process.nsolidDeps.* or adding a process.* would break the same people in the end. Actually, it's more dangerous to include a different type (object) to process.versions than to have all dependencies listed in the same way.

If we follow the process.nsolidDeps.* approach, we will also need to update node::Metadata to serialize node::Metadata::Versions differently, as it only expects std::string as keys. Maybe we should stick to the current implementation. What do you think?

@santigimeno
Copy link
Member

Agreed. Let's keep it this way then

@santigimeno
Copy link
Member

One final comment: I would add grpc, sodium and protobuf if possible. You're right they aren't direct deps, but they are updated separately from the direct dependency, which usually doesn't mandate a specific version.

@santigimeno
Copy link
Member

I added grpc support in cd89c1b

@RafaelGSS
Copy link
Member Author

It should all good now @santigimeno

RafaelGSS added a commit that referenced this pull request Sep 24, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
RafaelGSS added a commit that referenced this pull request Sep 24, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
RafaelGSS added a commit that referenced this pull request Sep 24, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
RafaelGSS added a commit that referenced this pull request Sep 24, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Sep 24, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
RafaelGSS added a commit that referenced this pull request Sep 24, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
RafaelGSS added a commit that referenced this pull request Sep 24, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
@RafaelGSS
Copy link
Member Author

Landed in cfd306f...0db40b4

1 similar comment
@RafaelGSS
Copy link
Member Author

Landed in cfd306f...0db40b4

@RafaelGSS RafaelGSS closed this Sep 24, 2024
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 10, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 10, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 10, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 10, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 10, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 10, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
trevnorris pushed a commit that referenced this pull request Oct 10, 2024
PR-URL: #166
Fixes: #163
Reviewed-By: Santiago Gimeno <[email protected]>
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.

N|Solid dependencies versions are not listed in process.versions nor in nsolid.info()
2 participants