GH-36456: [R] Link to correct version of OpenSSL when using autobrew - #36551

Merged
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl
Jul 16, 2023
Merged

GH-36456: [R] Link to correct version of OpenSSL when using autobrew#36551
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl

Conversation

@paleolimbot

@paleolimbotpaleolimbot commented Jul 7, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Rationale for this change

The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The ssl and crypto libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.

What changes are included in this PR?

This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds -lssl -lcrypto to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).

Are these changes tested?

Existing nightly tests cover these changes.

Are there any user-facing changes?

No.

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: b8962592d354f35dccf3684cf12ccb4618ef3cd5

Submitted crossbow builds: ursacomputing/crossbow @ actions-3d21794472

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: 15700d424518c86d0441705f1df078ae5a1ab713

Submitted crossbow builds: ursacomputing/crossbow @ actions-007d7f7198

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 10, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 10, 2023

Copy link
Copy Markdown

Revision: 6a538f442cd1223d84551ac876370ef2c286f978

Submitted crossbow builds: ursacomputing/crossbow @ actions-0bc60af847

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-bd672e75e9

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-47a81f0980

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot
paleolimbot marked this pull request as ready for review July 11, 2023 19:29
@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson If my reading of all this is correct, I think this will change the OpenSSL that is linked by default in the CRAN package that everybody uses. Is there a reason that this wasn't done before that I'm not aware of?

@nealrichardson

nealrichardson commented Jul 11, 2023•
edited
Loading

Copy link
Copy Markdown
Member

No reason I recall that it wasn't done before. How does upstream autobrew handle this?

If we're pinning a version, why not pin 3 instead of 1.1? (Edit: oh I see you mentioned that in the description)

@assignUserassignUser mentioned this pull request Jul 12, 2023
Closed
assignUser
assignUser approved these changes Jul 12, 2023
View reviewed changes

@assignUserassignUser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! I don't think it should be an issue for CRAN as we modify/don't use system libs explicitly in autobrew#L68-L75

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson Thanks!

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

kou
kou approved these changes Jul 12, 2023
View reviewed changes

@koukou left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Great!

(But I don't know why openssl@1.1 is chosen instead of openssl@3 by Apache Arrow C++ when both of them are installed...)

Comment threadr/tools/autobrew Outdated
@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 12, 2023
@nealrichardson

nealrichardson commented Jul 12, 2023

Copy link
Copy Markdown
Member

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

We do, but @jeroen maintains the actual autobrew formulae and scripts (here, here, and here, specifically), of which ours are copies that we modify to test on every commit. When changes to Homebrew happen or CRAN mandates new things, he handles it there, and we should watch (at release time, or when things break) for changes we need to pull in accordingly to stay in sync.

I don't see openssl pinned there, so I'm not sure what's different such that we need it--something with the self-hosted runner setup, perhaps? If so, that's probably ok, though might be worth adding comments that that's the case.

Re: openssl version, Homebrew actually has openssl@3: https://github.com/Homebrew/homebrew-core/blob/master/Formula/apache-arrow.rb#L30 If our copy is pinned on 1.1, we should update it, and pull in any other changes from upstream, same as autobrew.

@jeroen

jeroen commented Jul 12, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

You already explicitly depend on openssl via thrift (and aws-sdk maybe)?

## brew deps --tree apache-arrow-static
autobrew/cran/apache-arrow-static
├── aws-sdk-cpp-static
├── brotli
├── lz4
├── snappy
├── thrift
│ └── openssl@3
│ └── ca-certificates
└── zstd
├── lz4
└── xz

Upstream homebrew bumped thrift and aws-sdk to openssl@3 a few weeks ago, so the next release of the apache-arrow-static bundle will automatically include openssl@3.

The current PR will probably create a conflict because you end up depending on both openssl@11 and openssl@3. This may work in a dynamic linked stack, but once you try to statically link the entire stack in the R package this will probably create symbol conflicts.

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 12, 2023

Copy link
Copy Markdown

Revision: 018ca9807bc308cf6828bfdb7acd530ad28b1ef6

Submitted crossbow builds: ursacomputing/crossbow @ actions-225ba98cc6

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: ae451ee

Submitted crossbow builds: ursacomputing/crossbow @ actions-0fd077276c

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: 0043b1c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1602f52d96

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

Ok: I opened #36707 for the CI failure and for the r-binary-packages failure; also, I removed any unnecessary changes to the formulae.

kou
kou approved these changes Jul 16, 2023
View reviewed changes

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Comment threadr/configure

@koukouJul 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you revert changes in this file?

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 16, 2023
@paleolimbot
paleolimbot merged commit ed87a5b into apache:mainJul 16, 2023
@paleolimbotpaleolimbot removed the awaiting merge Awaiting merge label Jul 16, 2023
raulcd pushed a commit that referenced this pull request Jul 17, 2023
@paleolimbot@kou@raulcd
GH-36456: [R] Link to correct version of OpenSSL when using autobrew (#…
1d5f4cd
…36551)
### Rationale for this change
The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The `ssl` and `crypto` libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.
### What changes are included in this PR?
This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds `-lssl -lcrypto` to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).
### Are these changes tested?
Existing nightly tests cover these changes.
### Are there any user-facing changes?
No.
* Closes: #36456
Lead-authored-by: Dewey Dunnington <dewey@voltrondata.com>
Co-authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
@thisisnicthisisnic mentioned this pull request Jul 21, 2023
Closed
44 tasks
@conbench-apache-arrow

conbench-apache-arrowBot commented Jul 25, 2023

Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

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

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

Reviewers

@koukoukou approved these changes
@assignUserassignUserassignUser approved these changes
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees

No one assigned

Labels

Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[R] R package is linking to the wrong OpenSSL on MacOS BigSur?

6 participants

@paleolimbot@nealrichardson@jeroen@raulcd@kou@assignUser
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
Skip to content

GH-36456: [R] Link to correct version of OpenSSL when using autobrew - #36551

Merged
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl
Jul 16, 2023
Merged

GH-36456: [R] Link to correct version of OpenSSL when using autobrew#36551
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl

Conversation

@paleolimbot

@paleolimbotpaleolimbot commented Jul 7, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Rationale for this change

The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The ssl and crypto libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.

What changes are included in this PR?

This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds -lssl -lcrypto to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).

Are these changes tested?

Existing nightly tests cover these changes.

Are there any user-facing changes?

No.

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: b8962592d354f35dccf3684cf12ccb4618ef3cd5

Submitted crossbow builds: ursacomputing/crossbow @ actions-3d21794472

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: 15700d424518c86d0441705f1df078ae5a1ab713

Submitted crossbow builds: ursacomputing/crossbow @ actions-007d7f7198

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 10, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 10, 2023

Copy link
Copy Markdown

Revision: 6a538f442cd1223d84551ac876370ef2c286f978

Submitted crossbow builds: ursacomputing/crossbow @ actions-0bc60af847

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-bd672e75e9

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-47a81f0980

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot
paleolimbot marked this pull request as ready for review July 11, 2023 19:29
@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson If my reading of all this is correct, I think this will change the OpenSSL that is linked by default in the CRAN package that everybody uses. Is there a reason that this wasn't done before that I'm not aware of?

@nealrichardson

nealrichardson commented Jul 11, 2023•
edited
Loading

Copy link
Copy Markdown
Member

No reason I recall that it wasn't done before. How does upstream autobrew handle this?

If we're pinning a version, why not pin 3 instead of 1.1? (Edit: oh I see you mentioned that in the description)

@assignUserassignUser mentioned this pull request Jul 12, 2023
Closed
assignUser
assignUser approved these changes Jul 12, 2023
View reviewed changes

@assignUserassignUser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! I don't think it should be an issue for CRAN as we modify/don't use system libs explicitly in autobrew#L68-L75

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson Thanks!

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

kou
kou approved these changes Jul 12, 2023
View reviewed changes

@koukou left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Great!

(But I don't know why openssl@1.1 is chosen instead of openssl@3 by Apache Arrow C++ when both of them are installed...)

Comment threadr/tools/autobrew Outdated
@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 12, 2023
@nealrichardson

nealrichardson commented Jul 12, 2023

Copy link
Copy Markdown
Member

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

We do, but @jeroen maintains the actual autobrew formulae and scripts (here, here, and here, specifically), of which ours are copies that we modify to test on every commit. When changes to Homebrew happen or CRAN mandates new things, he handles it there, and we should watch (at release time, or when things break) for changes we need to pull in accordingly to stay in sync.

I don't see openssl pinned there, so I'm not sure what's different such that we need it--something with the self-hosted runner setup, perhaps? If so, that's probably ok, though might be worth adding comments that that's the case.

Re: openssl version, Homebrew actually has openssl@3: https://github.com/Homebrew/homebrew-core/blob/master/Formula/apache-arrow.rb#L30 If our copy is pinned on 1.1, we should update it, and pull in any other changes from upstream, same as autobrew.

@jeroen

jeroen commented Jul 12, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

You already explicitly depend on openssl via thrift (and aws-sdk maybe)?

## brew deps --tree apache-arrow-static
autobrew/cran/apache-arrow-static
├── aws-sdk-cpp-static
├── brotli
├── lz4
├── snappy
├── thrift
│ └── openssl@3
│ └── ca-certificates
└── zstd
├── lz4
└── xz

Upstream homebrew bumped thrift and aws-sdk to openssl@3 a few weeks ago, so the next release of the apache-arrow-static bundle will automatically include openssl@3.

The current PR will probably create a conflict because you end up depending on both openssl@11 and openssl@3. This may work in a dynamic linked stack, but once you try to statically link the entire stack in the R package this will probably create symbol conflicts.

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 12, 2023

Copy link
Copy Markdown

Revision: 018ca9807bc308cf6828bfdb7acd530ad28b1ef6

Submitted crossbow builds: ursacomputing/crossbow @ actions-225ba98cc6

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: ae451ee

Submitted crossbow builds: ursacomputing/crossbow @ actions-0fd077276c

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: 0043b1c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1602f52d96

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

Ok: I opened #36707 for the CI failure and for the r-binary-packages failure; also, I removed any unnecessary changes to the formulae.

kou
kou approved these changes Jul 16, 2023
View reviewed changes

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Comment threadr/configure

@koukouJul 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you revert changes in this file?

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 16, 2023
@paleolimbot
paleolimbot merged commit ed87a5b into apache:mainJul 16, 2023
@paleolimbotpaleolimbot removed the awaiting merge Awaiting merge label Jul 16, 2023
raulcd pushed a commit that referenced this pull request Jul 17, 2023
@paleolimbot@kou@raulcd
GH-36456: [R] Link to correct version of OpenSSL when using autobrew (#…
1d5f4cd
…36551)
### Rationale for this change
The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The `ssl` and `crypto` libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.
### What changes are included in this PR?
This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds `-lssl -lcrypto` to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).
### Are these changes tested?
Existing nightly tests cover these changes.
### Are there any user-facing changes?
No.
* Closes: #36456
Lead-authored-by: Dewey Dunnington <dewey@voltrondata.com>
Co-authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
@thisisnicthisisnic mentioned this pull request Jul 21, 2023
Closed
44 tasks
@conbench-apache-arrow

conbench-apache-arrowBot commented Jul 25, 2023

Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

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

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

Reviewers

@koukoukou approved these changes
@assignUserassignUserassignUser approved these changes
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees

No one assigned

Labels

Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[R] R package is linking to the wrong OpenSSL on MacOS BigSur?

6 participants

@paleolimbot@nealrichardson@jeroen@raulcd@kou@assignUser
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

GH-36456: [R] Link to correct version of OpenSSL when using autobrew - #36551

Merged
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl
Jul 16, 2023
Merged

GH-36456: [R] Link to correct version of OpenSSL when using autobrew#36551
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl

Conversation

@paleolimbot

@paleolimbotpaleolimbot commented Jul 7, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Rationale for this change

The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The ssl and crypto libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.

What changes are included in this PR?

This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds -lssl -lcrypto to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).

Are these changes tested?

Existing nightly tests cover these changes.

Are there any user-facing changes?

No.

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: b8962592d354f35dccf3684cf12ccb4618ef3cd5

Submitted crossbow builds: ursacomputing/crossbow @ actions-3d21794472

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: 15700d424518c86d0441705f1df078ae5a1ab713

Submitted crossbow builds: ursacomputing/crossbow @ actions-007d7f7198

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 10, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 10, 2023

Copy link
Copy Markdown

Revision: 6a538f442cd1223d84551ac876370ef2c286f978

Submitted crossbow builds: ursacomputing/crossbow @ actions-0bc60af847

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-bd672e75e9

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-47a81f0980

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot
paleolimbot marked this pull request as ready for review July 11, 2023 19:29
@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson If my reading of all this is correct, I think this will change the OpenSSL that is linked by default in the CRAN package that everybody uses. Is there a reason that this wasn't done before that I'm not aware of?

@nealrichardson

nealrichardson commented Jul 11, 2023•
edited
Loading

Copy link
Copy Markdown
Member

No reason I recall that it wasn't done before. How does upstream autobrew handle this?

If we're pinning a version, why not pin 3 instead of 1.1? (Edit: oh I see you mentioned that in the description)

@assignUserassignUser mentioned this pull request Jul 12, 2023
Closed
assignUser
assignUser approved these changes Jul 12, 2023
View reviewed changes

@assignUserassignUser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! I don't think it should be an issue for CRAN as we modify/don't use system libs explicitly in autobrew#L68-L75

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson Thanks!

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

kou
kou approved these changes Jul 12, 2023
View reviewed changes

@koukou left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Great!

(But I don't know why openssl@1.1 is chosen instead of openssl@3 by Apache Arrow C++ when both of them are installed...)

Comment threadr/tools/autobrew Outdated
@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 12, 2023
@nealrichardson

nealrichardson commented Jul 12, 2023

Copy link
Copy Markdown
Member

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

We do, but @jeroen maintains the actual autobrew formulae and scripts (here, here, and here, specifically), of which ours are copies that we modify to test on every commit. When changes to Homebrew happen or CRAN mandates new things, he handles it there, and we should watch (at release time, or when things break) for changes we need to pull in accordingly to stay in sync.

I don't see openssl pinned there, so I'm not sure what's different such that we need it--something with the self-hosted runner setup, perhaps? If so, that's probably ok, though might be worth adding comments that that's the case.

Re: openssl version, Homebrew actually has openssl@3: https://github.com/Homebrew/homebrew-core/blob/master/Formula/apache-arrow.rb#L30 If our copy is pinned on 1.1, we should update it, and pull in any other changes from upstream, same as autobrew.

@jeroen

jeroen commented Jul 12, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

You already explicitly depend on openssl via thrift (and aws-sdk maybe)?

## brew deps --tree apache-arrow-static
autobrew/cran/apache-arrow-static
├── aws-sdk-cpp-static
├── brotli
├── lz4
├── snappy
├── thrift
│ └── openssl@3
│ └── ca-certificates
└── zstd
├── lz4
└── xz

Upstream homebrew bumped thrift and aws-sdk to openssl@3 a few weeks ago, so the next release of the apache-arrow-static bundle will automatically include openssl@3.

The current PR will probably create a conflict because you end up depending on both openssl@11 and openssl@3. This may work in a dynamic linked stack, but once you try to statically link the entire stack in the R package this will probably create symbol conflicts.

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 12, 2023

Copy link
Copy Markdown

Revision: 018ca9807bc308cf6828bfdb7acd530ad28b1ef6

Submitted crossbow builds: ursacomputing/crossbow @ actions-225ba98cc6

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: ae451ee

Submitted crossbow builds: ursacomputing/crossbow @ actions-0fd077276c

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: 0043b1c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1602f52d96

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

Ok: I opened #36707 for the CI failure and for the r-binary-packages failure; also, I removed any unnecessary changes to the formulae.

kou
kou approved these changes Jul 16, 2023
View reviewed changes

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Comment threadr/configure

@koukouJul 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you revert changes in this file?

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 16, 2023
@paleolimbot
paleolimbot merged commit ed87a5b into apache:mainJul 16, 2023
@paleolimbotpaleolimbot removed the awaiting merge Awaiting merge label Jul 16, 2023
raulcd pushed a commit that referenced this pull request Jul 17, 2023
@paleolimbot@kou@raulcd
GH-36456: [R] Link to correct version of OpenSSL when using autobrew (#…
1d5f4cd
…36551)
### Rationale for this change
The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The `ssl` and `crypto` libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.
### What changes are included in this PR?
This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds `-lssl -lcrypto` to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).
### Are these changes tested?
Existing nightly tests cover these changes.
### Are there any user-facing changes?
No.
* Closes: #36456
Lead-authored-by: Dewey Dunnington <dewey@voltrondata.com>
Co-authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
@thisisnicthisisnic mentioned this pull request Jul 21, 2023
Closed
44 tasks
@conbench-apache-arrow

conbench-apache-arrowBot commented Jul 25, 2023

Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

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

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

Reviewers

@koukoukou approved these changes
@assignUserassignUserassignUser approved these changes
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees

No one assigned

Labels

Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[R] R package is linking to the wrong OpenSSL on MacOS BigSur?

6 participants

@paleolimbot@nealrichardson@jeroen@raulcd@kou@assignUser
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length \u003e 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

GH-36456: [R] Link to correct version of OpenSSL when using autobrew - #36551

Merged
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl
Jul 16, 2023
Merged

GH-36456: [R] Link to correct version of OpenSSL when using autobrew#36551
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl

Conversation

@paleolimbot

@paleolimbotpaleolimbot commented Jul 7, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Rationale for this change

The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The ssl and crypto libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.

What changes are included in this PR?

This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds -lssl -lcrypto to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).

Are these changes tested?

Existing nightly tests cover these changes.

Are there any user-facing changes?

No.

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: b8962592d354f35dccf3684cf12ccb4618ef3cd5

Submitted crossbow builds: ursacomputing/crossbow @ actions-3d21794472

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: 15700d424518c86d0441705f1df078ae5a1ab713

Submitted crossbow builds: ursacomputing/crossbow @ actions-007d7f7198

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 10, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 10, 2023

Copy link
Copy Markdown

Revision: 6a538f442cd1223d84551ac876370ef2c286f978

Submitted crossbow builds: ursacomputing/crossbow @ actions-0bc60af847

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-bd672e75e9

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-47a81f0980

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot
paleolimbot marked this pull request as ready for review July 11, 2023 19:29
@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson If my reading of all this is correct, I think this will change the OpenSSL that is linked by default in the CRAN package that everybody uses. Is there a reason that this wasn't done before that I'm not aware of?

@nealrichardson

nealrichardson commented Jul 11, 2023•
edited
Loading

Copy link
Copy Markdown
Member

No reason I recall that it wasn't done before. How does upstream autobrew handle this?

If we're pinning a version, why not pin 3 instead of 1.1? (Edit: oh I see you mentioned that in the description)

@assignUserassignUser mentioned this pull request Jul 12, 2023
Closed
assignUser
assignUser approved these changes Jul 12, 2023
View reviewed changes

@assignUserassignUser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! I don't think it should be an issue for CRAN as we modify/don't use system libs explicitly in autobrew#L68-L75

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson Thanks!

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

kou
kou approved these changes Jul 12, 2023
View reviewed changes

@koukou left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Great!

(But I don't know why openssl@1.1 is chosen instead of openssl@3 by Apache Arrow C++ when both of them are installed...)

Comment threadr/tools/autobrew Outdated
@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 12, 2023
@nealrichardson

nealrichardson commented Jul 12, 2023

Copy link
Copy Markdown
Member

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

We do, but @jeroen maintains the actual autobrew formulae and scripts (here, here, and here, specifically), of which ours are copies that we modify to test on every commit. When changes to Homebrew happen or CRAN mandates new things, he handles it there, and we should watch (at release time, or when things break) for changes we need to pull in accordingly to stay in sync.

I don't see openssl pinned there, so I'm not sure what's different such that we need it--something with the self-hosted runner setup, perhaps? If so, that's probably ok, though might be worth adding comments that that's the case.

Re: openssl version, Homebrew actually has openssl@3: https://github.com/Homebrew/homebrew-core/blob/master/Formula/apache-arrow.rb#L30 If our copy is pinned on 1.1, we should update it, and pull in any other changes from upstream, same as autobrew.

@jeroen

jeroen commented Jul 12, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

You already explicitly depend on openssl via thrift (and aws-sdk maybe)?

## brew deps --tree apache-arrow-static
autobrew/cran/apache-arrow-static
├── aws-sdk-cpp-static
├── brotli
├── lz4
├── snappy
├── thrift
│ └── openssl@3
│ └── ca-certificates
└── zstd
├── lz4
└── xz

Upstream homebrew bumped thrift and aws-sdk to openssl@3 a few weeks ago, so the next release of the apache-arrow-static bundle will automatically include openssl@3.

The current PR will probably create a conflict because you end up depending on both openssl@11 and openssl@3. This may work in a dynamic linked stack, but once you try to statically link the entire stack in the R package this will probably create symbol conflicts.

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 12, 2023

Copy link
Copy Markdown

Revision: 018ca9807bc308cf6828bfdb7acd530ad28b1ef6

Submitted crossbow builds: ursacomputing/crossbow @ actions-225ba98cc6

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: ae451ee

Submitted crossbow builds: ursacomputing/crossbow @ actions-0fd077276c

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: 0043b1c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1602f52d96

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

Ok: I opened #36707 for the CI failure and for the r-binary-packages failure; also, I removed any unnecessary changes to the formulae.

kou
kou approved these changes Jul 16, 2023
View reviewed changes

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Comment threadr/configure

@koukouJul 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you revert changes in this file?

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 16, 2023
@paleolimbot
paleolimbot merged commit ed87a5b into apache:mainJul 16, 2023
@paleolimbotpaleolimbot removed the awaiting merge Awaiting merge label Jul 16, 2023
raulcd pushed a commit that referenced this pull request Jul 17, 2023
@paleolimbot@kou@raulcd
GH-36456: [R] Link to correct version of OpenSSL when using autobrew (#…
1d5f4cd
…36551)
### Rationale for this change
The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The `ssl` and `crypto` libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.
### What changes are included in this PR?
This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds `-lssl -lcrypto` to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).
### Are these changes tested?
Existing nightly tests cover these changes.
### Are there any user-facing changes?
No.
* Closes: #36456
Lead-authored-by: Dewey Dunnington <dewey@voltrondata.com>
Co-authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
@thisisnicthisisnic mentioned this pull request Jul 21, 2023
Closed
44 tasks
@conbench-apache-arrow

conbench-apache-arrowBot commented Jul 25, 2023

Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

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

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

Reviewers

@koukoukou approved these changes
@assignUserassignUserassignUser approved these changes
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees

No one assigned

Labels

Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[R] R package is linking to the wrong OpenSSL on MacOS BigSur?

6 participants

@paleolimbot@nealrichardson@jeroen@raulcd@kou@assignUser
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

GH-36456: [R] Link to correct version of OpenSSL when using autobrew - #36551

Merged
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl
Jul 16, 2023
Merged

GH-36456: [R] Link to correct version of OpenSSL when using autobrew#36551
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl

Conversation

@paleolimbot

@paleolimbotpaleolimbot commented Jul 7, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Rationale for this change

The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The ssl and crypto libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.

What changes are included in this PR?

This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds -lssl -lcrypto to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).

Are these changes tested?

Existing nightly tests cover these changes.

Are there any user-facing changes?

No.

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: b8962592d354f35dccf3684cf12ccb4618ef3cd5

Submitted crossbow builds: ursacomputing/crossbow @ actions-3d21794472

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: 15700d424518c86d0441705f1df078ae5a1ab713

Submitted crossbow builds: ursacomputing/crossbow @ actions-007d7f7198

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 10, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 10, 2023

Copy link
Copy Markdown

Revision: 6a538f442cd1223d84551ac876370ef2c286f978

Submitted crossbow builds: ursacomputing/crossbow @ actions-0bc60af847

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-bd672e75e9

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-47a81f0980

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot
paleolimbot marked this pull request as ready for review July 11, 2023 19:29
@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson If my reading of all this is correct, I think this will change the OpenSSL that is linked by default in the CRAN package that everybody uses. Is there a reason that this wasn't done before that I'm not aware of?

@nealrichardson

nealrichardson commented Jul 11, 2023•
edited
Loading

Copy link
Copy Markdown
Member

No reason I recall that it wasn't done before. How does upstream autobrew handle this?

If we're pinning a version, why not pin 3 instead of 1.1? (Edit: oh I see you mentioned that in the description)

@assignUserassignUser mentioned this pull request Jul 12, 2023
Closed
assignUser
assignUser approved these changes Jul 12, 2023
View reviewed changes

@assignUserassignUser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! I don't think it should be an issue for CRAN as we modify/don't use system libs explicitly in autobrew#L68-L75

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson Thanks!

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

kou
kou approved these changes Jul 12, 2023
View reviewed changes

@koukou left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Great!

(But I don't know why openssl@1.1 is chosen instead of openssl@3 by Apache Arrow C++ when both of them are installed...)

Comment threadr/tools/autobrew Outdated
@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 12, 2023
@nealrichardson

nealrichardson commented Jul 12, 2023

Copy link
Copy Markdown
Member

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

We do, but @jeroen maintains the actual autobrew formulae and scripts (here, here, and here, specifically), of which ours are copies that we modify to test on every commit. When changes to Homebrew happen or CRAN mandates new things, he handles it there, and we should watch (at release time, or when things break) for changes we need to pull in accordingly to stay in sync.

I don't see openssl pinned there, so I'm not sure what's different such that we need it--something with the self-hosted runner setup, perhaps? If so, that's probably ok, though might be worth adding comments that that's the case.

Re: openssl version, Homebrew actually has openssl@3: https://github.com/Homebrew/homebrew-core/blob/master/Formula/apache-arrow.rb#L30 If our copy is pinned on 1.1, we should update it, and pull in any other changes from upstream, same as autobrew.

@jeroen

jeroen commented Jul 12, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

You already explicitly depend on openssl via thrift (and aws-sdk maybe)?

## brew deps --tree apache-arrow-static
autobrew/cran/apache-arrow-static
├── aws-sdk-cpp-static
├── brotli
├── lz4
├── snappy
├── thrift
│ └── openssl@3
│ └── ca-certificates
└── zstd
├── lz4
└── xz

Upstream homebrew bumped thrift and aws-sdk to openssl@3 a few weeks ago, so the next release of the apache-arrow-static bundle will automatically include openssl@3.

The current PR will probably create a conflict because you end up depending on both openssl@11 and openssl@3. This may work in a dynamic linked stack, but once you try to statically link the entire stack in the R package this will probably create symbol conflicts.

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 12, 2023

Copy link
Copy Markdown

Revision: 018ca9807bc308cf6828bfdb7acd530ad28b1ef6

Submitted crossbow builds: ursacomputing/crossbow @ actions-225ba98cc6

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: ae451ee

Submitted crossbow builds: ursacomputing/crossbow @ actions-0fd077276c

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: 0043b1c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1602f52d96

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

Ok: I opened #36707 for the CI failure and for the r-binary-packages failure; also, I removed any unnecessary changes to the formulae.

kou
kou approved these changes Jul 16, 2023
View reviewed changes

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Comment threadr/configure

@koukouJul 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you revert changes in this file?

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 16, 2023
@paleolimbot
paleolimbot merged commit ed87a5b into apache:mainJul 16, 2023
@paleolimbotpaleolimbot removed the awaiting merge Awaiting merge label Jul 16, 2023
raulcd pushed a commit that referenced this pull request Jul 17, 2023
@paleolimbot@kou@raulcd
GH-36456: [R] Link to correct version of OpenSSL when using autobrew (#…
1d5f4cd
…36551)
### Rationale for this change
The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The `ssl` and `crypto` libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.
### What changes are included in this PR?
This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds `-lssl -lcrypto` to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).
### Are these changes tested?
Existing nightly tests cover these changes.
### Are there any user-facing changes?
No.
* Closes: #36456
Lead-authored-by: Dewey Dunnington <dewey@voltrondata.com>
Co-authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
@thisisnicthisisnic mentioned this pull request Jul 21, 2023
Closed
44 tasks
@conbench-apache-arrow

conbench-apache-arrowBot commented Jul 25, 2023

Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

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

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

Reviewers

@koukoukou approved these changes
@assignUserassignUserassignUser approved these changes
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees

No one assigned

Labels

Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[R] R package is linking to the wrong OpenSSL on MacOS BigSur?

6 participants

@paleolimbot@nealrichardson@jeroen@raulcd@kou@assignUser
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

GH-36456: [R] Link to correct version of OpenSSL when using autobrew - #36551

Merged
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl
Jul 16, 2023
Merged

GH-36456: [R] Link to correct version of OpenSSL when using autobrew#36551
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl

Conversation

@paleolimbot

@paleolimbotpaleolimbot commented Jul 7, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Rationale for this change

The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The ssl and crypto libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.

What changes are included in this PR?

This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds -lssl -lcrypto to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).

Are these changes tested?

Existing nightly tests cover these changes.

Are there any user-facing changes?

No.

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: b8962592d354f35dccf3684cf12ccb4618ef3cd5

Submitted crossbow builds: ursacomputing/crossbow @ actions-3d21794472

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: 15700d424518c86d0441705f1df078ae5a1ab713

Submitted crossbow builds: ursacomputing/crossbow @ actions-007d7f7198

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 10, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 10, 2023

Copy link
Copy Markdown

Revision: 6a538f442cd1223d84551ac876370ef2c286f978

Submitted crossbow builds: ursacomputing/crossbow @ actions-0bc60af847

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-bd672e75e9

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-47a81f0980

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot
paleolimbot marked this pull request as ready for review July 11, 2023 19:29
@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson If my reading of all this is correct, I think this will change the OpenSSL that is linked by default in the CRAN package that everybody uses. Is there a reason that this wasn't done before that I'm not aware of?

@nealrichardson

nealrichardson commented Jul 11, 2023•
edited
Loading

Copy link
Copy Markdown
Member

No reason I recall that it wasn't done before. How does upstream autobrew handle this?

If we're pinning a version, why not pin 3 instead of 1.1? (Edit: oh I see you mentioned that in the description)

@assignUserassignUser mentioned this pull request Jul 12, 2023
Closed
assignUser
assignUser approved these changes Jul 12, 2023
View reviewed changes

@assignUserassignUser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! I don't think it should be an issue for CRAN as we modify/don't use system libs explicitly in autobrew#L68-L75

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson Thanks!

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

kou
kou approved these changes Jul 12, 2023
View reviewed changes

@koukou left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Great!

(But I don't know why openssl@1.1 is chosen instead of openssl@3 by Apache Arrow C++ when both of them are installed...)

Comment threadr/tools/autobrew Outdated
@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 12, 2023
@nealrichardson

nealrichardson commented Jul 12, 2023

Copy link
Copy Markdown
Member

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

We do, but @jeroen maintains the actual autobrew formulae and scripts (here, here, and here, specifically), of which ours are copies that we modify to test on every commit. When changes to Homebrew happen or CRAN mandates new things, he handles it there, and we should watch (at release time, or when things break) for changes we need to pull in accordingly to stay in sync.

I don't see openssl pinned there, so I'm not sure what's different such that we need it--something with the self-hosted runner setup, perhaps? If so, that's probably ok, though might be worth adding comments that that's the case.

Re: openssl version, Homebrew actually has openssl@3: https://github.com/Homebrew/homebrew-core/blob/master/Formula/apache-arrow.rb#L30 If our copy is pinned on 1.1, we should update it, and pull in any other changes from upstream, same as autobrew.

@jeroen

jeroen commented Jul 12, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

You already explicitly depend on openssl via thrift (and aws-sdk maybe)?

## brew deps --tree apache-arrow-static
autobrew/cran/apache-arrow-static
├── aws-sdk-cpp-static
├── brotli
├── lz4
├── snappy
├── thrift
│ └── openssl@3
│ └── ca-certificates
└── zstd
├── lz4
└── xz

Upstream homebrew bumped thrift and aws-sdk to openssl@3 a few weeks ago, so the next release of the apache-arrow-static bundle will automatically include openssl@3.

The current PR will probably create a conflict because you end up depending on both openssl@11 and openssl@3. This may work in a dynamic linked stack, but once you try to statically link the entire stack in the R package this will probably create symbol conflicts.

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 12, 2023

Copy link
Copy Markdown

Revision: 018ca9807bc308cf6828bfdb7acd530ad28b1ef6

Submitted crossbow builds: ursacomputing/crossbow @ actions-225ba98cc6

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: ae451ee

Submitted crossbow builds: ursacomputing/crossbow @ actions-0fd077276c

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: 0043b1c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1602f52d96

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

Ok: I opened #36707 for the CI failure and for the r-binary-packages failure; also, I removed any unnecessary changes to the formulae.

kou
kou approved these changes Jul 16, 2023
View reviewed changes

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Comment threadr/configure

@koukouJul 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you revert changes in this file?

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 16, 2023
@paleolimbot
paleolimbot merged commit ed87a5b into apache:mainJul 16, 2023
@paleolimbotpaleolimbot removed the awaiting merge Awaiting merge label Jul 16, 2023
raulcd pushed a commit that referenced this pull request Jul 17, 2023
@paleolimbot@kou@raulcd
GH-36456: [R] Link to correct version of OpenSSL when using autobrew (#…
1d5f4cd
…36551)
### Rationale for this change
The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The `ssl` and `crypto` libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.
### What changes are included in this PR?
This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds `-lssl -lcrypto` to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).
### Are these changes tested?
Existing nightly tests cover these changes.
### Are there any user-facing changes?
No.
* Closes: #36456
Lead-authored-by: Dewey Dunnington <dewey@voltrondata.com>
Co-authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
@thisisnicthisisnic mentioned this pull request Jul 21, 2023
Closed
44 tasks
@conbench-apache-arrow

conbench-apache-arrowBot commented Jul 25, 2023

Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

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

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

Reviewers

@koukoukou approved these changes
@assignUserassignUserassignUser approved these changes
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees

No one assigned

Labels

Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[R] R package is linking to the wrong OpenSSL on MacOS BigSur?

6 participants

@paleolimbot@nealrichardson@jeroen@raulcd@kou@assignUser
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

GH-36456: [R] Link to correct version of OpenSSL when using autobrew - #36551

Merged
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl
Jul 16, 2023
Merged

GH-36456: [R] Link to correct version of OpenSSL when using autobrew#36551
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl

Conversation

@paleolimbot

@paleolimbotpaleolimbot commented Jul 7, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Rationale for this change

The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The ssl and crypto libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.

What changes are included in this PR?

This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds -lssl -lcrypto to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).

Are these changes tested?

Existing nightly tests cover these changes.

Are there any user-facing changes?

No.

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: b8962592d354f35dccf3684cf12ccb4618ef3cd5

Submitted crossbow builds: ursacomputing/crossbow @ actions-3d21794472

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: 15700d424518c86d0441705f1df078ae5a1ab713

Submitted crossbow builds: ursacomputing/crossbow @ actions-007d7f7198

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 10, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 10, 2023

Copy link
Copy Markdown

Revision: 6a538f442cd1223d84551ac876370ef2c286f978

Submitted crossbow builds: ursacomputing/crossbow @ actions-0bc60af847

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-bd672e75e9

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-47a81f0980

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot
paleolimbot marked this pull request as ready for review July 11, 2023 19:29
@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson If my reading of all this is correct, I think this will change the OpenSSL that is linked by default in the CRAN package that everybody uses. Is there a reason that this wasn't done before that I'm not aware of?

@nealrichardson

nealrichardson commented Jul 11, 2023•
edited
Loading

Copy link
Copy Markdown
Member

No reason I recall that it wasn't done before. How does upstream autobrew handle this?

If we're pinning a version, why not pin 3 instead of 1.1? (Edit: oh I see you mentioned that in the description)

@assignUserassignUser mentioned this pull request Jul 12, 2023
Closed
assignUser
assignUser approved these changes Jul 12, 2023
View reviewed changes

@assignUserassignUser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! I don't think it should be an issue for CRAN as we modify/don't use system libs explicitly in autobrew#L68-L75

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson Thanks!

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

kou
kou approved these changes Jul 12, 2023
View reviewed changes

@koukou left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Great!

(But I don't know why openssl@1.1 is chosen instead of openssl@3 by Apache Arrow C++ when both of them are installed...)

Comment threadr/tools/autobrew Outdated
@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 12, 2023
@nealrichardson

nealrichardson commented Jul 12, 2023

Copy link
Copy Markdown
Member

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

We do, but @jeroen maintains the actual autobrew formulae and scripts (here, here, and here, specifically), of which ours are copies that we modify to test on every commit. When changes to Homebrew happen or CRAN mandates new things, he handles it there, and we should watch (at release time, or when things break) for changes we need to pull in accordingly to stay in sync.

I don't see openssl pinned there, so I'm not sure what's different such that we need it--something with the self-hosted runner setup, perhaps? If so, that's probably ok, though might be worth adding comments that that's the case.

Re: openssl version, Homebrew actually has openssl@3: https://github.com/Homebrew/homebrew-core/blob/master/Formula/apache-arrow.rb#L30 If our copy is pinned on 1.1, we should update it, and pull in any other changes from upstream, same as autobrew.

@jeroen

jeroen commented Jul 12, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

You already explicitly depend on openssl via thrift (and aws-sdk maybe)?

## brew deps --tree apache-arrow-static
autobrew/cran/apache-arrow-static
├── aws-sdk-cpp-static
├── brotli
├── lz4
├── snappy
├── thrift
│ └── openssl@3
│ └── ca-certificates
└── zstd
├── lz4
└── xz

Upstream homebrew bumped thrift and aws-sdk to openssl@3 a few weeks ago, so the next release of the apache-arrow-static bundle will automatically include openssl@3.

The current PR will probably create a conflict because you end up depending on both openssl@11 and openssl@3. This may work in a dynamic linked stack, but once you try to statically link the entire stack in the R package this will probably create symbol conflicts.

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 12, 2023

Copy link
Copy Markdown

Revision: 018ca9807bc308cf6828bfdb7acd530ad28b1ef6

Submitted crossbow builds: ursacomputing/crossbow @ actions-225ba98cc6

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: ae451ee

Submitted crossbow builds: ursacomputing/crossbow @ actions-0fd077276c

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: 0043b1c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1602f52d96

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

Ok: I opened #36707 for the CI failure and for the r-binary-packages failure; also, I removed any unnecessary changes to the formulae.

kou
kou approved these changes Jul 16, 2023
View reviewed changes

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Comment threadr/configure

@koukouJul 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you revert changes in this file?

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 16, 2023
@paleolimbot
paleolimbot merged commit ed87a5b into apache:mainJul 16, 2023
@paleolimbotpaleolimbot removed the awaiting merge Awaiting merge label Jul 16, 2023
raulcd pushed a commit that referenced this pull request Jul 17, 2023
@paleolimbot@kou@raulcd
GH-36456: [R] Link to correct version of OpenSSL when using autobrew (#…
1d5f4cd
…36551)
### Rationale for this change
The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The `ssl` and `crypto` libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.
### What changes are included in this PR?
This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds `-lssl -lcrypto` to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).
### Are these changes tested?
Existing nightly tests cover these changes.
### Are there any user-facing changes?
No.
* Closes: #36456
Lead-authored-by: Dewey Dunnington <dewey@voltrondata.com>
Co-authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
@thisisnicthisisnic mentioned this pull request Jul 21, 2023
Closed
44 tasks
@conbench-apache-arrow

conbench-apache-arrowBot commented Jul 25, 2023

Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

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

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

Reviewers

@koukoukou approved these changes
@assignUserassignUserassignUser approved these changes
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees

No one assigned

Labels

Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[R] R package is linking to the wrong OpenSSL on MacOS BigSur?

6 participants

@paleolimbot@nealrichardson@jeroen@raulcd@kou@assignUser
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

GH-36456: [R] Link to correct version of OpenSSL when using autobrew - #36551

Merged
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl
Jul 16, 2023
Merged

GH-36456: [R] Link to correct version of OpenSSL when using autobrew#36551
paleolimbot merged 17 commits into
apache:mainfrom
paleolimbot:r-openssl

Conversation

@paleolimbot

@paleolimbotpaleolimbot commented Jul 7, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Rationale for this change

The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The ssl and crypto libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.

What changes are included in this PR?

This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds -lssl -lcrypto to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).

Are these changes tested?

Existing nightly tests cover these changes.

Are there any user-facing changes?

No.

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: b8962592d354f35dccf3684cf12ccb4618ef3cd5

Submitted crossbow builds: ursacomputing/crossbow @ actions-3d21794472

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 7, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 7, 2023

Copy link
Copy Markdown

Revision: 15700d424518c86d0441705f1df078ae5a1ab713

Submitted crossbow builds: ursacomputing/crossbow @ actions-007d7f7198

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 10, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 10, 2023

Copy link
Copy Markdown

Revision: 6a538f442cd1223d84551ac876370ef2c286f978

Submitted crossbow builds: ursacomputing/crossbow @ actions-0bc60af847

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-bd672e75e9

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 11, 2023

Copy link
Copy Markdown

Revision: 047bc6afcb085c736ad2b29d4b369e4e74963432

Submitted crossbow builds: ursacomputing/crossbow @ actions-47a81f0980

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot
paleolimbot marked this pull request as ready for review July 11, 2023 19:29
@paleolimbot

paleolimbot commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson If my reading of all this is correct, I think this will change the OpenSSL that is linked by default in the CRAN package that everybody uses. Is there a reason that this wasn't done before that I'm not aware of?

@nealrichardson

nealrichardson commented Jul 11, 2023•
edited
Loading

Copy link
Copy Markdown
Member

No reason I recall that it wasn't done before. How does upstream autobrew handle this?

If we're pinning a version, why not pin 3 instead of 1.1? (Edit: oh I see you mentioned that in the description)

@assignUserassignUser mentioned this pull request Jul 12, 2023
Closed
assignUser
assignUser approved these changes Jul 12, 2023
View reviewed changes

@assignUserassignUser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! I don't think it should be an issue for CRAN as we modify/don't use system libs explicitly in autobrew#L68-L75

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@nealrichardson Thanks!

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

kou
kou approved these changes Jul 12, 2023
View reviewed changes

@koukou left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Great!

(But I don't know why openssl@1.1 is chosen instead of openssl@3 by Apache Arrow C++ when both of them are installed...)

Comment threadr/tools/autobrew Outdated
@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 12, 2023
@nealrichardson

nealrichardson commented Jul 12, 2023

Copy link
Copy Markdown
Member

How does upstream autobrew handle this?

I am not sure what this means! I assumed the formula in the autobrew repos is modified to reflect the one I modified here before each release? My understanding of autobrew is admittedly limited.

We do, but @jeroen maintains the actual autobrew formulae and scripts (here, here, and here, specifically), of which ours are copies that we modify to test on every commit. When changes to Homebrew happen or CRAN mandates new things, he handles it there, and we should watch (at release time, or when things break) for changes we need to pull in accordingly to stay in sync.

I don't see openssl pinned there, so I'm not sure what's different such that we need it--something with the self-hosted runner setup, perhaps? If so, that's probably ok, though might be worth adding comments that that's the case.

Re: openssl version, Homebrew actually has openssl@3: https://github.com/Homebrew/homebrew-core/blob/master/Formula/apache-arrow.rb#L30 If our copy is pinned on 1.1, we should update it, and pull in any other changes from upstream, same as autobrew.

@jeroen

jeroen commented Jul 12, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

You already explicitly depend on openssl via thrift (and aws-sdk maybe)?

## brew deps --tree apache-arrow-static
autobrew/cran/apache-arrow-static
├── aws-sdk-cpp-static
├── brotli
├── lz4
├── snappy
├── thrift
│ └── openssl@3
│ └── ca-certificates
└── zstd
├── lz4
└── xz

Upstream homebrew bumped thrift and aws-sdk to openssl@3 a few weeks ago, so the next release of the apache-arrow-static bundle will automatically include openssl@3.

The current PR will probably create a conflict because you end up depending on both openssl@11 and openssl@3. This may work in a dynamic linked stack, but once you try to statically link the entire stack in the R package this will probably create symbol conflicts.

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 12, 2023

Copy link
Copy Markdown

Revision: 018ca9807bc308cf6828bfdb7acd530ad28b1ef6

Submitted crossbow builds: ursacomputing/crossbow @ actions-225ba98cc6

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: ae451ee

Submitted crossbow builds: ursacomputing/crossbow @ actions-0fd077276c

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit r-binary-packages homebrew-r-autobrew

@github-actions

github-actionsBot commented Jul 16, 2023

Copy link
Copy Markdown

Revision: 0043b1c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1602f52d96

TaskStatus
homebrew-r-autobrewGithub Actions
r-binary-packagesGithub Actions

@paleolimbot

paleolimbot commented Jul 16, 2023

Copy link
Copy Markdown
MemberAuthor

Ok: I opened #36707 for the CI failure and for the r-binary-packages failure; also, I removed any unnecessary changes to the formulae.

kou
kou approved these changes Jul 16, 2023
View reviewed changes

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Comment threadr/configure

@koukouJul 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you revert changes in this file?

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 16, 2023
@paleolimbot
paleolimbot merged commit ed87a5b into apache:mainJul 16, 2023
@paleolimbotpaleolimbot removed the awaiting merge Awaiting merge label Jul 16, 2023
raulcd pushed a commit that referenced this pull request Jul 17, 2023
@paleolimbot@kou@raulcd
GH-36456: [R] Link to correct version of OpenSSL when using autobrew (#…
1d5f4cd
…36551)
### Rationale for this change
The r-binary-packages job (which uses autobrew) and the autobrew nightly jobs are failing because they are linking to a different version of OpenSSL than the package was built against. I believe this occurred because Arrow and its dependencies are built against the autobrew headers which included openssl. The `ssl` and `crypto` libraries weren't explicitly linked, so I think whatever LibreSSL fork MacOS installs by default was getting linked. This was perhaps compatible using the version of autobrew for High Sierra/the version of LibreSSL on High Sierra but was not compatible with the version of autobrew for Big Sur/the version of LibreSSL on Big Sur.
### What changes are included in this PR?
This PR explicitly adds OpenSSL 1.1 to the autobrew formulas and explicitly adds `-lssl -lcrypto` to the PKG_LIBS (1.1 because that's what was in the corresponding homebrew formula).
### Are these changes tested?
Existing nightly tests cover these changes.
### Are there any user-facing changes?
No.
* Closes: #36456
Lead-authored-by: Dewey Dunnington <dewey@voltrondata.com>
Co-authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
@thisisnicthisisnic mentioned this pull request Jul 21, 2023
Closed
44 tasks
@conbench-apache-arrow

conbench-apache-arrowBot commented Jul 25, 2023

Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

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

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

Reviewers

@koukoukou approved these changes
@assignUserassignUserassignUser approved these changes
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees

No one assigned

Labels

Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[R] R package is linking to the wrong OpenSSL on MacOS BigSur?

6 participants

@paleolimbot@nealrichardson@jeroen@raulcd@kou@assignUser