GH-39523: [R] Don't override explicitly set NOT_CRAN=false when on dev version by assignUser · Pull Request #39524 · apache/arrow

assignUser · 2024-01-09T01:26:11Z

Rationale for this change

The default linux build used in the lto job should not build with aws/gcs. A change in the build system changed this.

What changes are included in this PR?

Revert to old behavior by not overriding explicitly set NOT_CRAN=false.

Are these changes tested?

CI

Are there any user-facing changes?

No

Closes: [R][CI] lto nightly fails because s2n-tls can't be build with lto #39523

assignUser · 2024-01-09T01:28:05Z

@github-actions crossbow submit test-r-rhub-debian-gcc-devel-lto-latest

github-actions · 2024-01-09T01:30:16Z

Revision: 56a3caca91c8d93e920ae141248d08dd9360cb81

Submitted crossbow builds: ursacomputing/crossbow @ actions-7790c7b4b9

Task	Status
test-r-rhub-debian-gcc-devel-lto-latest

assignUser · 2024-01-09T02:08:44Z

It seems our azure queue is pretty long again (13) and this is the last in line. I have run the job locally using archery and it succeeds:

─  checking whether package ‘arrow’ can be installed ... [1755s/588s] OK (9m 48.2s)
─  used C++ compiler: ‘g++ (Debian 13.2.0-7) 13.2.0’
✔  checking C++ specification
     Not all R platforms support C++17
N  checking installed package size ...
     installed size is 104.8Mb
     sub-directories of 1Mb or more:
       libs  99.9Mb
       R      4.3Mb
✔  checking package directory

cpp/cmake_modules/ThirdpartyToolchain.cmake

assignUser · 2024-01-09T03:30:38Z

That works as well:

─  checking whether package ‘arrow’ can be installed ... [1688s/596s] OK (9m 55.9s)
─  used C++ compiler: ‘g++ (Debian 13.2.0-7) 13.2.0’
✔  checking C++ specification
     Not all R platforms support C++17
N  checking installed package size ...
     installed size is 104.8Mb
     sub-directories of 1Mb or more:
       libs  99.9Mb
       R      4.3Mb

kou · 2024-01-09T03:43:32Z

@github-actions crossbow submit test-r-rhub-debian-gcc-devel-lto-latest

github-actions · 2024-01-09T03:45:50Z

Revision: 0193cb97bc5629a51087b058dc62cdaaf67621f4

Submitted crossbow builds: ursacomputing/crossbow @ actions-278d36119d

Task	Status
test-r-rhub-debian-gcc-devel-lto-latest

assignUser · 2024-01-09T04:36:30Z

hm, thats weird ... I'll test with main to confirm it fails locally. @kou anyidea why it didn't work?

kou · 2024-01-09T05:23:04Z

I could reproduce this by R_ORG=rhub R_IMAGE=debian-gcc-devel-lto R_TAG=latest archery docker run -e NOT_CRAN=false -e INSTALL_ARGS=--use-LTO r on local.
I'll take a look at this.

assignUser · 2024-01-09T05:35:22Z

For me it passes on main as well... :/ Thanks for taking a look!

kou · 2024-01-09T05:44:57Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

Sorry. We need REGEX:

Suggested change

string(REPLACE "-flto[^ ]*" "" AWS_LC_C_FLAGS "${AWS_LC_C_FLAGS}")

string(REGEX REPLACE "-flto[^ ]*" "" AWS_LC_C_FLAGS "${AWS_LC_C_FLAGS}")

But there is another error:

** testing if installed package can be loaded from temporary location Error: package or namespace load failed for ‘arrow’ in dyn.load(file, DLLpath = DLLpath, ...): unable to load shared object '/arrow/r/check/arrow.Rcheck/00LOCK-arrow/00new/arrow/libs/arrow.so': /arrow/r/check/arrow.Rcheck/00LOCK-arrow/00new/arrow/libs/arrow.so: undefined symbol: OPENSSL_free

This warning may be related:

g++ -std=gnu++17 -shared -Wall -pedantic -flto -fpic -L/opt/R-devel/lib/R/lib -L/usr/local/lib -o arrow.so RTasks.o altrep.o array.o array_to_vector.o arraydata.o arrowExports.o bridge.o buffer.o chunkedarray.o compression.o compute-exec.o compute.o config.o csv.o dataset.o datatype.o expression.o extension-impl.o feather.o field.o filesystem.o io.o json.o memorypool.o message.o parquet.o r_to_arrow.o recordbatch.o recordbatchreader.o recordbatchwriter.o safe-call-into-r-impl.o scalar.o schema.o symbols.o table.o threadpool.o type_infer.o -L/arrow/r/check/arrow.Rcheck/00_pkg_src/arrow/libarrow/arrow-14.0.1.100000264/lib -L/usr/lib/lib/x86_64-linux-gnu -L/usr/lib/lib -larrow_dataset -lparquet -larrow_acero -larrow -larrow_bundled_dependencies /usr/lib/x86_64-linux-gnu/libbz2.so -lbrotlidec -lbrotlienc -lz -lzstd -lcurl -lssl -lcrypto -L/opt/R-devel/lib/R/lib -lR lto-wrapper: warning: Extra option to ‘-Xassembler’: --noexecstack, dropping all ‘-Xassembler’ and ‘-Wa’ options. /tmp/RtmpoBLPp7/working_dir/RtmpRJJbvI/file77c6759da9b/aws_lc_ep-install/include/openssl/err.h:176: warning: type of ‘ERR_get_error’ does not match original declaration [-Wlto-type-mismatch] /usr/include/openssl/err.h:417: note: return value type mismatch 417 | unsigned long ERR_get_error(void); | /usr/include/openssl/err.h:417: note: type ‘long unsigned int’ should match type ‘uint32_t’ /usr/include/openssl/err.h:417: note: ‘ERR_get_error’ was previously declared here /usr/include/openssl/err.h:417: note: code may be misoptimized unless ‘-fno-strict-aliasing’ is used lto-wrapper: warning: using serial compilation of 128 LTRANS jobs lto-wrapper: note: see the ‘-flto’ option documentation for more information

OpenSSL 1 and 3 are mixed?

Hmm, OPENSSL_free() is a macro both in OpenSSL 1.1 and 3.
Is a wrong header file used?

aws-lc's openssl/mem.h defines OPENSSL_free() as a function...

oh wait, I think I know why this is happening, let me check.

assignUser · 2024-01-09T22:17:13Z

Yeah, sorry this is my fault, in the build refactor I also set not_cran=true if we are on a dev version and it overrides explicitly set envvar. So the issues is that this was never supposed to build like this. I will revert these changes and fix the issue in nixlibs.R. (this is also no longer a blocker)

assignUser · 2024-01-09T22:25:44Z

@github-actions crossbow submit test-r-rhub-debian-gcc-devel-lto-latest

github-actions · 2024-01-10T17:53:42Z

Revision: 1795586

Submitted crossbow builds: ursacomputing/crossbow @ actions-89de4b20e3

Task	Status
test-r-rhub-debian-gcc-devel-lto-latest

r/tools/nixlibs.R

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

kou

+1

conbench-apache-arrow · 2024-01-13T10:24:08Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 3cc04f1.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

… on dev version (apache#39524) ### Rationale for this change The default linux build used in the lto job should not build with aws/gcs. A change in the build system changed this. ### What changes are included in this PR? Revert to old behavior by not overriding explicitly set `NOT_CRAN=false`. ### Are these changes tested? CI ### Are there any user-facing changes? No * Closes: apache#39523 Lead-authored-by: Jacob Wujciak-Jens <jacob@wujciak.de> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>

…v version (#39524) ### Rationale for this change The default linux build used in the lto job should not build with aws/gcs. A change in the build system changed this. ### What changes are included in this PR? Revert to old behavior by not overriding explicitly set `NOT_CRAN=false`. ### Are these changes tested? CI ### Are there any user-facing changes? No * Closes: #39523 Lead-authored-by: Jacob Wujciak-Jens <jacob@wujciak.de> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>

github-actions bot added Component: C++ awaiting review Awaiting review labels Jan 9, 2024

assignUser requested a review from kou January 9, 2024 01:27

kou reviewed Jan 9, 2024

View reviewed changes

cpp/cmake_modules/ThirdpartyToolchain.cmake Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Jan 9, 2024

assignUser requested a review from kou January 9, 2024 03:30

kou reviewed Jan 9, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jan 9, 2024

Don't override explicitly set NOT_CRAN in nixlibs.R

2499939

assignUser force-pushed the no-lto-s2n branch from 0193cb9 to 2499939 Compare January 9, 2024 22:23

github-actions bot added Component: R awaiting change review Awaiting change review and removed Component: C++ awaiting changes Awaiting changes labels Jan 9, 2024

assignUser changed the title ~~GH-39523: [C++] Remove additional variations of the lto flag for s2n-tls~~ GH-39523: [R] Don't override explicitly set NOT_CRAN=false when on dev version Jan 9, 2024

This comment was marked as outdated.

Sign in to view

fix logic

1795586

assignUser requested a review from kou January 11, 2024 03:35

kou reviewed Jan 11, 2024

View reviewed changes

r/tools/nixlibs.R Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jan 11, 2024

Update r/tools/nixlibs.R

d4aed04

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jan 11, 2024

kou approved these changes Jan 12, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Jan 12, 2024

assignUser merged commit 3cc04f1 into apache:main Jan 12, 2024

assignUser removed the awaiting merge Awaiting merge label Jan 12, 2024

	string(REPLACE "-flto[^ ]*" "" AWS_LC_C_FLAGS "${AWS_LC_C_FLAGS}")
	string(REGEX REPLACE "-flto[^ ]*" "" AWS_LC_C_FLAGS "${AWS_LC_C_FLAGS}")

Conversation

assignUser commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

assignUser commented Jan 9, 2024

Uh oh!

github-actions bot commented Jan 9, 2024

Uh oh!

assignUser commented Jan 9, 2024

Uh oh!

Uh oh!

assignUser commented Jan 9, 2024

Uh oh!

kou commented Jan 9, 2024

Uh oh!

github-actions bot commented Jan 9, 2024

Uh oh!

assignUser commented Jan 9, 2024

Uh oh!

kou commented Jan 9, 2024

Uh oh!

assignUser commented Jan 9, 2024

Uh oh!

kou Jan 9, 2024

Choose a reason for hiding this comment

Uh oh!

kou Jan 9, 2024

Choose a reason for hiding this comment

Uh oh!

kou Jan 9, 2024

Choose a reason for hiding this comment

Uh oh!

kou Jan 9, 2024

Choose a reason for hiding this comment

Uh oh!

kou Jan 9, 2024

Choose a reason for hiding this comment

Uh oh!

assignUser Jan 9, 2024

Choose a reason for hiding this comment

Uh oh!

assignUser commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

assignUser commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

github-actions bot commented Jan 10, 2024

Uh oh!

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

conbench-apache-arrow bot commented Jan 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

assignUser commented Jan 9, 2024 •

edited

Loading

assignUser commented Jan 9, 2024 •

edited

Loading

assignUser commented Jan 9, 2024 •

edited

Loading