Skip to content

Test this repo against grpc/grpc-node#1239#21702

Closed
murgatroid99 wants to merge 2 commits intogrpc:masterfrom
murgatroid99:node_repo_test_fixes
Closed

Test this repo against grpc/grpc-node#1239#21702
murgatroid99 wants to merge 2 commits intogrpc:masterfrom
murgatroid99:node_repo_test_fixes

Conversation

@murgatroid99
Copy link
Copy Markdown
Member

Background: The Node test jobs in this repo's PRs run this script in the node repository, passing a commit reference for the commit in the PR. That script is missing a set -e line and recently the command to check out the submodule commit has been failing because of some changes made to the third_party/upb. As a result of that combination of problems, the script has been silently testing the current state of the grpc-node repository, instead of the state corresponding to updating the submodule with the contents of the PR.

This PR runs the Node test job against grpc/grpc-node#1239, which fixes those problems. There are other known integration problems, so the Node test job for this PR will fail initially.

@nicolasnoble
Copy link
Copy Markdown
Contributor

So, @veblush, the node test was broken here reporting no problem (see grpc/grpc-node#1239 - the upb submodule change broke it), which means a few build issues cropped since this happened. One of them being this:

static library binding.gyp:absl__strings__strings#target has several files with the same basename:
  escaping: deps/grpc/third_party/abseil-cpp/absl/strings/escaping.cc deps/grpc/third_party/abseil-cpp/absl/strings/internal/escaping.cc

This probably is something we want to double-file with abseil. The gist is that if you try to create a static library under macos using XCode with the same filename twice, one will get ignored or overridden by the other one, even if they come from different sub directories.

I am guessing this issue with escaping.cc isn't biting us right now because we're currently not using features from these files, but nodejs' build system is more pedantic to make sure this doesn't spread. #21639 has more details on what's going on.

@nicolasnoble
Copy link
Copy Markdown
Contributor

Hopefully we can get abseil/abseil-cpp#605 in, to fix the next breakage on this PR.

@veblush
Copy link
Copy Markdown
Contributor

veblush commented Jan 17, 2020

Thank you for the report. This is interesting but I think it could be a bit out-dated warning. The warning was needed before with msvc2008 and mac make with libtool on mac and it has been considered to remove this check as no one is actively using those. Is it possible to use --no-duplicate-basename-check ?

Reference:

@nicolasnoble
Copy link
Copy Markdown
Contributor

@veblush: if this was working, sure: nodejs/node-gyp#2026

@stale
Copy link
Copy Markdown

stale bot commented May 6, 2020

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 30 days. It will be closed automatically if no further update occurs in 7 day. Thank you for your contributions!

@stale stale bot closed this May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants