-
Notifications
You must be signed in to change notification settings - Fork 3.8k
GH-46974: [Integration][Archery] Add support for ARROW_JS_ROOT #46975
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
Conversation
|
ARROW_JS_ROOT = os.path.join(ARROW_BUILD_ROOT, 'js') | ||
ARROW_JS_ROOT = os.environ.get( | ||
'ARROW_JS_ROOT', | ||
os.path.join(ARROW_BUILD_ROOT, 'js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even need the ARROW_BUILD_ROOT
fallback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove it.
cc0056e
to
4130e21
Compare
4130e21
to
788fd97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've validated that ARROW_BUILD_ROOT
is still required for the tester_java.py
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 2de43af. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Rationale for this change
If we can specify the JS implementation directory by an environment variable, it's useful. Because developers don't need to copy the JS implementation to
apache/arrow/js
.What changes are included in this PR?
Refer
ARROW_JS_ROOT
before using the default JS implementation directory.Are these changes tested?
Yes.
Are there any user-facing changes?
No.