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: repo/path mismatch in v8 update #465

Merged
merged 1 commit into from
Aug 6, 2020

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Aug 6, 2020

Closes nodejs/node-v8#166.

It appears that the latest version of V8 has stripped the v8/ prefix out of pathing, so e.g in DEPS:

  'v8/build':
    Var('chromium_url') + '/chromium/src/build.git' + '@' + '1b904cc30093c25d5fd48389bd58e3f7409bcf80',
  'v8/third_party/depot_tools':
    Var('chromium_url') + '/chromium/tools/depot_tools.git' + '@' + '454f4ba4b3a69feb03c73f93d789062033433b4c',
  'v8/third_party/icu':
    Var('chromium_url') + '/chromium/deps/icu.git' + '@' + 'f2223961702f00a8833874b0560d615a2cc42738',
  'v8/third_party/instrumented_libraries':
    Var('chromium_url') + '/chromium/src/third_party/instrumented_libraries.git' + '@' + 'bb3f1802c237dd19105dd0f7919f99e536a39d10',
  'v8/buildtools':
    Var('chromium_url') + '/chromium/src/buildtools.git' + '@' + '204a35a2a64f7179f8b76d7a0385653690839e21',
  'v8/buildtools/clang_format/script':
    Var('chromium_url') + '/chromium/llvm-project/cfe/tools/clang-format.git' + '@' + '96636aa0e9f047f17447f2d45a094d0b59ed7917',
  'v8/buildtools/linux64': {
    'packages': [
      {
        'package': 'gn/gn/linux-amd64',
        'version': Var('gn_version'),
      }
    ],
    'dep_type': 'cipd',
    'condition': 'host_os == "linux"',
  },

is now:

  'build':
    Var('chromium_url') + '/chromium/src/build.git' + '@' + '1b904cc30093c25d5fd48389bd58e3f7409bcf80',
  'third_party/depot_tools':
    Var('chromium_url') + '/chromium/tools/depot_tools.git' + '@' + '454f4ba4b3a69feb03c73f93d789062033433b4c',
  'tthird_party/icu':
    Var('chromium_url') + '/chromium/deps/icu.git' + '@' + 'f2223961702f00a8833874b0560d615a2cc42738',
  'third_party/instrumented_libraries':
    Var('chromium_url') + '/chromium/src/third_party/instrumented_libraries.git' + '@' + 'bb3f1802c237dd19105dd0f7919f99e536a39d10',
  'buildtools':
    Var('chromium_url') + '/chromium/src/buildtools.git' + '@' + '204a35a2a64f7179f8b76d7a0385653690839e21',
  'buildtools/clang_format/script':
    Var('chromium_url') + '/chromium/llvm-project/cfe/tools/clang-format.git' + '@' + '96636aa0e9f047f17447f2d45a094d0b59ed7917',
  'buildtools/linux64': {
    'packages': [
      {
        'package': 'gn/gn/linux-amd64',
        'version': Var('gn_version'),
      }
    ],
    'dep_type': 'cipd',
    'condition': 'host_os == "linux"',
  },

Tested with: git-node v8 major --no-version-bump on latest Node.js master

cc @mmarchini @gengjiawen

@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #465 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #465   +/-   ##
=======================================
  Coverage   76.88%   76.88%           
=======================================
  Files          28       28           
  Lines        1752     1752           
=======================================
  Hits         1347     1347           
  Misses        405      405           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48c306b...ed864ce. Read the comment docs.

@mmarchini
Copy link
Contributor

We might need to consider both paths because we use the same command to bump on release branches (I think we'll need the older paths for at least one more release, although I might be wrong).

@gengjiawen
Copy link
Member

gengjiawen commented Aug 6, 2020

We might need to consider both paths because we use the same command to bump on release branches (I think we'll need the older paths for at least one more release, although I might be wrong).

will an extra flag for compatibility work ? Also, cc @targos

Update: @targos use v8 version to fix prefix in https://github.com/nodejs/node-core-utils/pull/465/files

@gengjiawen
Copy link
Member

Look into this a bit more, the prefix still in v8 8.5 https://github.com/v8/v8/blob/8.5-lkgr/DEPS.
V8 8.5 is WIP by @targos for Node.js core.

Copy link
Contributor

@mmarchini mmarchini left a comment

Choose a reason for hiding this comment

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

LGTM with a follow up PR to address previous versions

@codebytere
Copy link
Member Author

  1) auth
       asks for auth data if no ncurc is found:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/node-core-utils/node-core-utils/test/unit/auth.test.js)
  

  2) auth
       asks for auth data if ncurc is invalid json:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/node-core-utils/node-core-utils/test/unit/auth.test.js)

are failing but passing locally so think this is good to merge

@priyank-p
Copy link
Contributor

I vaguely remember those test being flaky so this is good to merge. I triggered a re-run though.

@priyank-p
Copy link
Contributor

(Test passed!)

@codebytere codebytere merged commit 57b7df8 into nodejs:master Aug 6, 2020
@codebytere codebytere deleted the fix-v8-major-bump branch August 6, 2020 17:25
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.

V8 update script broken
4 participants