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

Fix latest ArgumentException for C# extensions #6938

Merged
merged 6 commits into from Dec 5, 2019

Conversation

ObsidianMinor
Copy link
Contributor

@ObsidianMinor ObsidianMinor commented Nov 24, 2019

Adds a Distinct filter for extensions loaded from depended descriptors. Fixes #6936

cc: @jtattermusch @rafi-kamal

{
registry.AddRange(dependencies.SelectMany(GetAllDependedExtensions).Concat(GetAllGeneratedExtensions(generatedInfo)).ToArray());
return dependencies.SelectMany(GetAllDependedExtensions).Distinct().Concat(GetAllGeneratedExtensions(generatedInfo));
Copy link
Contributor Author

@ObsidianMinor ObsidianMinor Nov 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we get an enumerable over the files and run distinct on those so we don't duplicate work by loading the file's extensions multiple times?

@rafi-kamal rafi-kamal added c# kokoro:run release notes: yes labels Nov 24, 2019
@rafi-kamal
Copy link
Contributor

@rafi-kamal rafi-kamal commented Nov 24, 2019

The MacOS Ruby test failure is a known one (thanks for fixing it so quickly btw).

@anandolee anandolee requested a review from jtattermusch Nov 25, 2019
@jtattermusch jtattermusch self-assigned this Nov 26, 2019
csharp/protos/extensions/extensions_b.proto Outdated Show resolved Hide resolved
csharp/protos/extensions/extensions_b.proto Outdated Show resolved Hide resolved
csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs Outdated Show resolved Hide resolved
csharp/protos/extensions/extensions_a.proto Outdated Show resolved Hide resolved
csharp/protos/extensions/extensions_a.proto Outdated Show resolved Hide resolved
src/google/protobuf/unittest_well_known_types.proto \
src/google/protobuf/test_messages_proto3.proto \
src/google/protobuf/test_messages_proto2.proto
map_unittest_proto3.proto \
Copy link
Contributor

@jtattermusch jtattermusch Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you removing the prefix from the path? Looks unrelated to this PR.

Copy link
Contributor

@jtattermusch jtattermusch Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert unnecessary changes.

@jtattermusch
Copy link
Contributor

@jtattermusch jtattermusch commented Dec 3, 2019

distcheck is currently failing (see test results).

Copy link
Contributor

@jtattermusch jtattermusch left a comment

LGTM once the tests pass.

@ObsidianMinor thanks for fixing the problem!

@jtattermusch jtattermusch merged commit c8a5634 into protocolbuffers:master Dec 5, 2019
55 of 56 checks passed
rafi-kamal pushed a commit that referenced this issue Feb 7, 2020
Fix latest ArgumentException for C# extensions
rafi-kamal added a commit that referenced this issue Feb 10, 2020
Fix latest ArgumentException for C# extensions

Co-authored-by: Jan Tattermusch <jtattermusch@users.noreply.github.com>
owent added a commit to owent-contrib/protobuf that referenced this issue Apr 14, 2020
…eep the code to remove repeated depended files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c# cla: yes release notes: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants