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.

Sign up for GitHub

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

Jump to bottom

tools: add multiple locations support in license builder #39361

Closed
wants to merge 1 commit into from
Closed

tools: add multiple locations support in license builder #39361

wants to merge 1 commit into from

Conversation

legendecas
Copy link
Member

Refs: #39360

@nodejs-github-bot nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Jul 12, 2021
@legendecas legendecas force-pushed the tools/license-builder branch from 639c3d5 to 792d095 Compare July 12, 2021 16:10
@legendecas
Copy link
Member Author

@aduh95 hi, since you edited this file recently, may I have your review on this PR? Many thanks!

richardlau
richardlau requested changes Jul 13, 2021
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

I missed whatever PR added it but I'd prefer we didn't have duplication in the repository... I'm a bit surprised that gtest is in src.

Even if we allow for the duplication, we should do basic checks that the licenses in each location are actually the same.

@legendecas
Copy link
Member Author

@richardlau the reason that we split a part of gtest into src is that the only file in src/gtest is a header only gtest_prod.h and it contains no actual code generation but only class friendship declaration (for cctest).

Do you suggest we should check the LICENSE files src/gtest/LICENSE and test/cctest/gtest/LICENSE in tools/license-builder.sh?

@targos
Copy link
Member

targos commented Jul 13, 2021

There's also a copy of gtest_prod.h in deps/v8. Could we use it?

@legendecas
Copy link
Member Author

@targos There's also a copy of gtest_prod.h in deps/v8. Could we use it?

It can work. But that would cause indirect dependencies IIUC. There's also a copy of zlib in deps/v8 yet we use the copy in deps/zlib instead.

@targos
Copy link
Member

targos commented Jul 14, 2021

@legendecas good point. Then could we put the file in test/cctest/gtest/gtest_prod.h instead of src?

@legendecas
Copy link
Member Author

@targos only header gtest_prod.h can be referenced in src. Other header files located in test/cctest/gtest should not be referenced in src as they include testing implementations. Though I don't think it is a good idea to referencing test/* headers from src. Or we can move all of them to deps/googletest so that we don't need two separate places for gtest.

@targos
Copy link
Member

targos commented Jul 14, 2021

we can move all of them to deps/googletest so that we don't need two separate places for gtest.

SGTM

@legendecas legendecas mentioned this pull request Jul 14, 2021
Closed
@legendecas
Copy link
Member Author

closed in favor of #39386

@legendecas legendecas closed this Jul 14, 2021
legendecas added a commit that referenced this pull request Jul 16, 2021
@legendecas
deps: extract gtest source files to deps/googletest
575266a 
PR-URL: #39386
Refs: #39361
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jul 17, 2021
@legendecas @targos
deps: extract gtest source files to deps/googletest
494c2c1 
PR-URL: #39386
Refs: #39361
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@legendecas legendecas deleted the tools/license-builder branch July 18, 2021 14:01
BethGriggs pushed a commit that referenced this pull request Jul 29, 2021
@legendecas @BethGriggs
deps: extract gtest source files to deps/googletest
626eb07 
PR-URL: #39386
Refs: #39361
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Sep 4, 2021
@legendecas @targos
deps: extract gtest source files to deps/googletest
4c93968 
PR-URL: #39386
Refs: #39361
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau requested changes

@aduh95 aduh95 aduh95 approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@legendecas @targos @jasnell @richardlau @aduh95 @nodejs-github-bot