Skip to content

ARROW-14510: [R] [CI] ensure that docker runs don't use host-built artifacts #11944

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
Closed
jonkeane wants to merge 3 commits into from
Closed

ARROW-14510: [R] [CI] ensure that docker runs don't use host-built artifacts #11944

jonkeane wants to merge 3 commits into from

Conversation

@jonkeane
Copy link
Member

@jonkeane jonkeane commented Dec 13, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented Dec 13, 2021

https://issues.apache.org/jira/browse/ARROW-14510

@jonkeane
Copy link
Member Author

jonkeane commented Dec 13, 2021

@github-actions crossbow submit test-ubuntu-18.04-r-sanitizer test-fedora-r-clang-sanitizer test-r-linux-valgrind test-ubuntu-default-docs

@github-actions
Copy link

github-actions bot commented Dec 13, 2021

Revision: f37cc99

Submitted crossbow builds: ursacomputing/crossbow @ actions-1291

Task Status
test-fedora-r-clang-sanitizer Azure
test-r-linux-valgrind Azure
test-ubuntu-18.04-r-sanitizer Azure
test-ubuntu-default-docs Azure

@jonkeane
Copy link
Member Author

jonkeane commented Dec 13, 2021

@github-actions crossbow submit test-ubuntu-18.04-r-sanitizer test-fedora-r-clang-sanitizer test-r-linux-valgrind test-ubuntu-default-docs

@github-actions
Copy link

github-actions bot commented Dec 13, 2021

Revision: 450f251

Submitted crossbow builds: ursacomputing/crossbow @ actions-1292

Task Status
test-fedora-r-clang-sanitizer Azure
test-r-linux-valgrind Azure
test-ubuntu-18.04-r-sanitizer Azure
test-ubuntu-default-docs Azure

@jonkeane
Copy link
Member Author

jonkeane commented Dec 13, 2021

@github-actions crossbow submit test-ubuntu-18.04-r-sanitizer test-fedora-r-clang-sanitizer test-r-linux-valgrind test-ubuntu-default-docs

@github-actions
Copy link

github-actions bot commented Dec 13, 2021

Revision: 01b8dc7

Submitted crossbow builds: ursacomputing/crossbow @ actions-1293

Task Status
test-fedora-r-clang-sanitizer Azure
test-r-linux-valgrind Azure
test-ubuntu-18.04-r-sanitizer Azure
test-ubuntu-default-docs Azure

@jonkeane jonkeane changed the title ARROW-14510: [R] run make clean when building docs ARROW-14510: [R] ensure that docker runs don't use host-built artifacts Dec 14, 2021
@jonkeane jonkeane changed the title ARROW-14510: [R] ensure that docker runs don't use host-built artifacts ARROW-14510: [R] [CI] ensure that docker runs don't use host-built artifacts Dec 14, 2021
@jonkeane
Copy link
Member Author

jonkeane commented Dec 14, 2021

The two sanitizer failures are legit (and not caused by this ticket — they look like they are from #11850)

thisisnic
thisisnic approved these changes Dec 14, 2021
View reviewed changes
Copy link
Member

@thisisnic thisisnic left a comment

Choose a reason for hiding this comment

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

One comment, otherwise LGTM

ci/scripts/r_valgrind.sh

popd

popd
Copy link
Member

@thisisnic thisisnic Dec 14, 2021

Choose a reason for hiding this comment

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

Is this a typo or is it needed again here?

Copy link
Member

@rok rok Dec 14, 2021

Choose a reason for hiding this comment

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

Are popds even needed here? My assumption is shell scripts don't influence the state after completing.

Copy link
Member Author

@jonkeane jonkeane Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I added another pushd up above, so I added this down here to match the flow — but you're right that this shouldn't change the state or anything so is probably not needed.

rok
rok reviewed Dec 14, 2021
View reviewed changes
Copy link
Member

@rok rok left a comment

Choose a reason for hiding this comment

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

LGTM!

Out of curiosity - wouldn't ${R_BIN} CMD INSTALL --clean ${INSTALL_ARGS} as suggested here also solve this problem? (Sorry for my ignorance if this is a completely off mark.)

ci/scripts/r_valgrind.sh

popd

popd
Copy link
Member

@rok rok Dec 14, 2021

Choose a reason for hiding this comment

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

Are popds even needed here? My assumption is shell scripts don't influence the state after completing.

@jonkeane
Copy link
Member Author

jonkeane commented Dec 14, 2021

Re: ${R_BIN} CMD INSTALL --clean ${INSTALL_ARGS} That should solve the problem (though reading WRE it was't 100% clear if the ./cleanup file needed to be up to date with all the files that needed to be removed (in which case it could still fail if it's not up to date for some reason) or if --clean would do that on it's own). But more importantly: that would remove the built artifacts from the directory which might force a re-compile elsewhere. This fix intentionally tried to not impact the shared directory between docker and the host so that if the developer relies on keeping the artifacts around between builds, those will not be overwritten or deleted.

@rok
Copy link
Member

rok commented Dec 14, 2021

Re: ${R_BIN} CMD INSTALL --clean ${INSTALL_ARGS} That should solve the problem (though reading WRE it was't 100% clear if the ./cleanup file needed to be up to date with all the files that needed to be removed (in which case it could still fail if it's not up to date for some reason) or if --clean would do that on it's own). But more importantly: that would remove the built artifacts from the directory which might force a re-compile elsewhere. This fix intentionally tried to not impact the shared directory between docker and the host so that if the developer relies on keeping the artifacts around between builds, those will not be overwritten or deleted.

Got it! That makes sense, thanks for the explanation :).

@jonkeane jonkeane closed this in acce836 Dec 17, 2021
@ursabot
Copy link

ursabot commented Dec 17, 2021 •
edited
Loading

Benchmark runs are scheduled for baseline = 7cf7442 and contender = acce836. acce836 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rok rok rok left review comments

@thisisnic thisisnic thisisnic 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.

4 participants

@jonkeane @rok @ursabot @thisisnic