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(builtin): linker test should run program as an action #1113

Merged
merged 1 commit into from
Sep 10, 2019

Conversation

alexeagle
Copy link
Collaborator

We were accidentally using runfiles resolution to read all user inputs, because we ran as an sh_test

@buildsize
Copy link

buildsize bot commented Sep 9, 2019

File name Previous Size New Size Change
release.tar.gz 1.02 MB 1.03 MB 7.43 KB (1%)

@alexeagle alexeagle force-pushed the linker branch 12 times, most recently from 3e186e2 to 9cb8218 Compare September 10, 2019 13:09
We were accidentally using runfiles resolution to read all user inputs, because we ran as an sh_test
Copy link
Collaborator

@gregmagolan gregmagolan left a comment

Choose a reason for hiding this comment

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

Amazing!

internal/linker/link_node_modules.ts Outdated Show resolved Hide resolved
# However, we still want to be able to write our tooling in TypeScript.
# This macro lets us check in the resulting .js files, and still ensure that they are
# compiled from the .ts by using a golden file test.
def checked_in_ts_library(name, checked_in_js, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

what ends up copying the output of the ts_library here to the check_in_js file? maybe I'm just not seeing it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's the golden_file_test below

// Use junction on Windows since symlinks require elevated permissions.
// We only link to directories so junctions work for us.
fs.symlinkSync(target, path, 'junction');
if (VERBOSE_LOGS) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe if (VERBOSE_LOGS || DEBUG) so if you set DEBUG env you're checking for this? Maybe throw in that case as well since seems like failure you should find out about if you set DEBUG?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't want to throw because I think it's probably wrong for the program to have different behavior when you turn on VERBOSE_LOGS - seems like we want a strict mode in general which is maybe different from how much logging we do

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking just throw if DEBUG was set not on VERBOSE_LOGS. Program can have different behaviors with DEBUG.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But maybe a strict mode would make more sense instead of overloading the DEBUG semantics

@alexeagle alexeagle merged commit 7f0102e into bazel-contrib:master Sep 10, 2019
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