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

Prepare bazel integration tests #17304

Open
wants to merge 22 commits into
base: master
from

Conversation

@filipesilva
Copy link
Member

filipesilva commented Mar 25, 2020

Groundwork for #16976

@filipesilva filipesilva requested a review from clydin Mar 25, 2020
@googlebot googlebot added the cla: yes label Mar 25, 2020
@filipesilva filipesilva force-pushed the filipesilva:prepare-bazel-integration-tests branch 2 times, most recently from 89223a1 to 78cae69 Mar 25, 2020
@@ -28,7 +28,7 @@ const basicFile = stripIndent`
"x-bar": 5,
}`;

const representativeFile = readFileSync(__dirname + '/test/angular.json', 'utf8');
const representativeFile = readFileSync(require.resolve(__dirname + '/test/angular.json'), 'utf8');

This comment has been minimized.

Copy link
@clydin

clydin Mar 25, 2020

Member

__dirname is an absolute path. require.resolve should be a noop in this case. Can you provide some background on what's happening here on windows?

This comment has been minimized.

Copy link
@filipesilva

filipesilva Mar 26, 2020

Author Member

In general, any sort of node file lookup on Bazel should be subject to require.resolve. This is how rules_nodejs resolves paths using the Bazel runfiles mechanism, where a given Bazel target only has access to outputs from its dependencies.

In practice, this does not make a lot of difference on Linux. A symlink forest is laid down where the target is going to actually run, and mostly the files are resolved correctly whether you use require.resolve or not because the files are there.

On windows though, that's a stricter. Bazel does not lay down a symlink forest on windows by default. If you don't use require.resolve, it's still possible to correctly resolve some files, like outputs from other rules. But other files, like node modules dependencies and data files, need to be looked up in the runfiles.

Since the requirement is quite lax on Linux but quite strict on windows, what ends up happening is that lack of require.resolve calls go unnoticed until someone tries to run things on Windows. Then it breaks.

This comment has been minimized.

Copy link
@clydin

clydin Mar 26, 2020

Member

Ah. ok. so this is really a workaround for Bazel Windows support?

Since this is not needed on any OS outside of Bazel nor with Bazel on linux/Mac, might be good to add a comment so someone reading through knows why it's there and doesn't try to remove it unintentionally.

This comment has been minimized.

Copy link
@alan-agius4

alan-agius4 Mar 31, 2020

Collaborator

@filipesilva, this doesn't seem correct for Windows either. Under Bazel to resolve files from the manifest require.resolve need to contain a full path including the workspace name, similar to rlocation.

Example;

require.resolve('workspace_name/tsconfig.json');

Will resolve the file from manifest to an absolute path:

C:/git/workspace_name/tsconfig.json

In the above case, the path provided will never be in the MANIFEST, because Bazel paths/keys are never absolute.

MANIFEST extract

angular_bazel_architect/src/polyfills.ts C:/git/rules_nodejs/examples/angular_bazel_architect/src/polyfills.ts
angular_bazel_architect/src/styles.scss C:/git/rules_nodejs/examples/angular_bazel_architect/src/styles.scss
angular_bazel_architect/tsconfig.app.json C:/git/rules_nodejs/examples/angular_bazel_architect/tsconfig.app.json
angular_bazel_architect/tsconfig.json C:/git/rules_nodejs/examples/angular_bazel_architect/tsconfig.json

This comment has been minimized.

Copy link
@filipesilva

filipesilva Mar 31, 2020

Author Member

That's a good point. I wonder why this worked then... I assumed the paths were in the manifest, but didn't verify.

I know rules_nodejs' require.resolve has fallbacks. The absolute path must hit the fallback then? But then this still gets confusing because the data files can only be looked up by runfiles...

Went looking at the require patches in rules_nodejs and found this:
https://github.com/bazelbuild/rules_nodejs/blob/cd8520d870abe490d7a154f7e1d9ccac32ea04d8/internal/node/require_patch.js#L141-L148

  // Determine bin and gen root to convert absolute paths into runfile paths.
  const binRootIdx = manifestPath.indexOf(BIN_DIR);
  let binRoot, genRoot;
  if (binRootIdx !== -1) {
    const execRoot = manifestPath.slice(0, binRootIdx);
    binRoot = `${execRoot}${BIN_DIR}/`;
    genRoot = `${execRoot}${GEN_DIR}/`;
  }

So that makes sense now: the absolute paths are converted.

This comment has been minimized.

Copy link
@alan-agius4

alan-agius4 Mar 31, 2020

Collaborator

Thanks for checking @filipesilva.

Here's where the conversion happens:
https://github.com/bazelbuild/rules_nodejs/blob/cd8520d870abe490d7a154f7e1d9ccac32ea04d8/internal/node/require_patch.js#L307-L315

        runfilesEntry.startsWith(binRoot) || runfilesEntry.startsWith(genRoot) ||
        runfilesEntry.startsWith(localWorkspacePath)) {
      // For absolute paths, replace binRoot, genRoot or localWorkspacePath with
      // USER_WORKSPACE_NAME to enable lookups.
      // It's OK to do multiple replacements because all of these are absolute paths with drive
      // names (e.g. C:\), and on Windows you can't have drive names in the middle of paths.
      runfilesEntry = runfilesEntry.replace(binRoot, `${USER_WORKSPACE_NAME}/`)
                          .replace(genRoot, `${USER_WORKSPACE_NAME}/`)
                          .replace(localWorkspacePath, `${USER_WORKSPACE_NAME}/`);
packages/ngtools/webpack/BUILD Outdated Show resolved Hide resolved
"verdaccio": "4.5.1"
"verdaccio": "4.5.1",
"webpack": "4.42.0",
"webpack-sources": "1.4.3"

This comment has been minimized.

Copy link
@clydin

clydin Mar 25, 2020

Member

Having to put all the dependencies in two places is really awkward and a potential maintenance burden. Hopefully this can be rectified in the future.

This comment has been minimized.

Copy link
@filipesilva

filipesilva Mar 26, 2020

Author Member

At its core, this is needed because rules_nodejs doesn't understand yarn workspaces. It's not super clear to me whether it should though, or how yarn pnp plays into that either.

We could reduce this burden with some health checks on our repository, like checking that these versions are the same everywhere they are mentioned, plus some exceptions. Maybe at that point what we need to to also loop in dev-infra to share that tooling between fw/comp/cli. We're the only ones that use yarn workspaces afaik but we're not the only ones that have versions sprinkled around that need to be matched.

This comment has been minimized.

Copy link
@clydin

clydin Mar 26, 2020

Member

For now, I think extra validation checks are going to be paramount. Especially to ensure that the tests are using the same dependency versions that the users are using when they install the package.

I think this is also problematic if we need to use a different version of a dependency for a particular package (recent build optimizer TS issue, for example) or is there a mechanism for that situation? I don't think there are any of these currently though.

This comment has been minimized.

Copy link
@filipesilva

This comment has been minimized.

Copy link
@alan-agius4

alan-agius4 Mar 31, 2020

Collaborator

We should also update renovate configuration now, so that it doesn't create separate PRs for dependencies and dev deps, as otherwise we'll have 2 PRs for each dependency update.

// TODO: This folder should not be prefixed with `not-`, but its's needed to workaround a Bazel bug:
// https://github.com/bazelbuild/rules_nodejs/issues/313
export { NodePackageInstallTask } from './not-node-package/install-task';
export { NodePackageLinkTask } from './not-node-package/link-task';

This comment has been minimized.

Copy link
@clydin

clydin Mar 25, 2020

Member

This is quite invasive. Is there any chance we can prioritize a fix for this upstream?

This comment has been minimized.

Copy link
@filipesilva

filipesilva Mar 26, 2020

Author Member

Looping in @gregmagolan to see what he thinks.

packages/angular_devkit/schematics_cli/BUILD.bazel Outdated Show resolved Hide resolved
filipesilva added 21 commits Jan 29, 2020
…UMDs

The TS bug is microsoft/TypeScript#36780.

The workaround is needed because `ts_library` emits UMDs currently. This will change with bazelbuild/rules_typescript#492 and bazelbuild/rules_nodejs#1687.
Using an extension gives editors a chance to figure out the correct language syntax to use.
This PR changes ts_json_schema to not create a ts_library rule for the json schema, and instead let consuming ts_libraries use and compile the resulting .ts files themselves.

This is needed in order to maintain the module_name of the consuming libraries on the .ts files resulting from json compilation.
Without this change, module names will be computed with incorrect paths containing duplicate path fragments (e.g. `@angular-devkit/architect/testing/testing/test-logger`).
@filipesilva filipesilva force-pushed the filipesilva:prepare-bazel-integration-tests branch from 78cae69 to 0d5f938 Mar 26, 2020
The build-webpack large spec uses it, but it was toplevel only because it was hoisted.
@filipesilva filipesilva force-pushed the filipesilva:prepare-bazel-integration-tests branch from 0d5f938 to e38d9b1 Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.