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

feat: support source files with duplicate basename #62

Merged
merged 2 commits into from
Sep 9, 2020
Merged

feat: support source files with duplicate basename #62

merged 2 commits into from
Sep 9, 2020

Conversation

targos
Copy link
Member

@targos targos commented Sep 7, 2020

This change removes the duplicate basename checks for macOS and MSVS.
The macOS one is no longer necessary and the solution generator for MSVS
is updated to support duplicate names by reproducing the source
directory structure in the intermediate directory.

Closes: #60

BREAKING CHANGE:

The --no-duplicate-basename-check option was removed.

@targos
feat: support source files with duplicate basename
bba609e 
This change removes the duplicate basename checks for macOS and MSVS.
The macOS one is no longer necessary and the solution generator for MSVS
is updated to support duplicate names by reproducing the source
directory structure in the intermediate directory.

Closes: nodejs#60

BREAKING CHANGE:

The `--no-duplicate-basename-check` option was removed.
@targos
Copy link
Member Author

targos commented Sep 7, 2020

/cc @gengjiawen

@targos targos marked this pull request as draft September 7, 2020 09:11
@targos
Copy link
Member Author

targos commented Sep 7, 2020

Converted to draft because it doesn't work yet with Node.js:

  snapshot-empty.cc
     Création de la bibliothèque ..\..\out\Release\mksnapshot.lib et de l'objet ..\..\out\Release\mksnapshot.exp
  mksnapshot.vcxproj -> ..\..\out\Release\\mksnapshot.exe
  generating: "..\..\out\Release\obj\v8_snapshot\/snapshot.cc" "..\..\out\Release\obj\v8_snapshot\/embedded.S"
  Assembling ..\..\out\Release\obj\v8_snapshot\\embedded.S...
  setup-isolate-deserialize.cc
  snapshot.cc
c1xx : fatal error C1083: Impossible d'ouvrir le fichier source : '..\..\out\Release\obj\v8_snapshot\\snapshot.cc' : No such file or directory [C:\git\nodejs\node\tools\v8_gypfiles\v8    
_snapshot.vcxproj]

@targos
Copy link
Member Author

targos commented Sep 7, 2020

Found a bug, now recompiling node from scratch...

@targos
Copy link
Member Author

targos commented Sep 7, 2020

Build passed!

@targos targos marked this pull request as ready for review September 7, 2020 10:29
@gengjiawen gengjiawen mentioned this pull request Sep 7, 2020
Closed
@gengjiawen
Copy link
Member

Is it possible we add some end to end test for this ?

@targos
Copy link
Member Author

targos commented Sep 7, 2020

Probably. I have never looked at how tests are written in this project though.

@cclauss
Copy link
Contributor

cclauss commented Sep 7, 2020

I have never looked at how tests are written

GitHub Actions runs pytest...

Test with pytest
========================== 29 passed in 0.41 seconds ===========================
Run pytest
============================= test session starts ==============================
platform darwin -- Python 2.7.18, pytest-4.6.11, py-1.9.0, pluggy-0.13.1
rootdir: /Users/runner/work/gyp-next/gyp-next
collected 29 items

pylib/gyp/MSVSSettings_test.py ........                                  [ 27%]
pylib/gyp/common_test.py ....                                            [ 41%]
pylib/gyp/easy_xml_test.py .....                                         [ 58%]
pylib/gyp/input_test.py .......                                          [ 82%]
pylib/gyp/generator/msvs_test.py .                                       [ 86%]
pylib/gyp/generator/ninja_test.py ..                                     [ 93%]
pylib/gyp/generator/xcode_test.py ..                                     [100%]

========================== 29 passed in 0.41 seconds ===========================

gengjiawen
gengjiawen approved these changes Sep 7, 2020
View reviewed changes
Copy link
Member

@gengjiawen gengjiawen left a comment

Choose a reason for hiding this comment

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

LGTM

cclauss
cclauss approved these changes Sep 7, 2020
View reviewed changes
Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

LGTM

@gengjiawen gengjiawen merged commit 72eddfe into nodejs:master Sep 9, 2020
@targos targos deleted the support-duplicate-basename branch September 9, 2020 09:39
@gengjiawen gengjiawen mentioned this pull request Sep 16, 2020
Closed
@github-actions github-actions bot mentioned this pull request Mar 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gengjiawen gengjiawen gengjiawen approved these changes

@cclauss cclauss cclauss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support same file name on windows
3 participants
@targos @gengjiawen @cclauss