Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-37510: [C++] Don't install bundled Azure SDK for C++ #38176

Merged
merged 3 commits into from
Oct 11, 2023

Conversation

kou
Copy link
Member

@kou kou commented Oct 10, 2023

Rationale for this change

It's an internal bundled library. We should not install it as a part of Arrow.

What changes are included in this PR?

Exclude all Azure SDK for C++ jobs including install jobs aren't executed by default. Building jobs are executed because they are required to build Arrow.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

@github-actions
Copy link

⚠️ GitHub issue #37510 has been automatically assigned in GitHub to PR creator.

Copy link
Member

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

With the release imminent and there being a clear workaroudn (system azure sdk) I am ok with this solution.

A future improvement might be installing the sdk into the build tree instead of the host system by modifing:

        RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR} # DLLs (if produced by build) go to "/bin"
        LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} # static .lib files
        ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} # .lib files for DLL build
        INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}

Or setting EXCLUDE_FROM_ALL for each of the azure targets might also work? Not sure if dir scope is needed to supress the installation...

Comment on lines +5085 to +5086
# EXCLUDE_FROM_ALL is available since CMake 3.28
# EXCLUDE_FROM_ALL TRUE
Copy link
Member

Choose a reason for hiding this comment

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

iirc you could just leave that there and cmake will ignore it if it doesn't know it

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, CMake reports an error for this case...

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 11, 2023
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 11, 2023
@kou
Copy link
Member Author

kou commented Oct 11, 2023

A future improvement might be installing the sdk into the build tree instead of the host system by modifing:

Does this approach apply a patch to Azure SDK for C++? I want to avoid having a patch...

Or setting EXCLUDE_FROM_ALL for each of the azure targets might also work? Not sure if dir scope is needed to suppress the installation...

It will work but I want to maintain target list for Azure SDK for C++... It'll increase maintenance cost like Abseil...

@kou
Copy link
Member Author

kou commented Oct 11, 2023

I'll merge this for 14.0.0.

@kou kou merged commit 7047e63 into apache:main Oct 11, 2023
49 checks passed
@kou kou deleted the cpp-azure-sdk-cpp-no-install branch October 11, 2023 20:38
@kou kou removed the awaiting change review Awaiting change review label Oct 11, 2023
raulcd pushed a commit that referenced this pull request Oct 12, 2023
### Rationale for this change

It's an internal bundled library. We should not install it as a part of Arrow.

### What changes are included in this PR?

Exclude all Azure SDK for C++ jobs including install jobs aren't executed by default. Building jobs are executed because they are required to build Arrow.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: #37510

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
llama90 pushed a commit to llama90/arrow that referenced this pull request Oct 12, 2023
…#38176)

### Rationale for this change

It's an internal bundled library. We should not install it as a part of Arrow.

### What changes are included in this PR?

Exclude all Azure SDK for C++ jobs including install jobs aren't executed by default. Building jobs are executed because they are required to build Arrow.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: apache#37510

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@conbench-apache-arrow
Copy link

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

There were no benchmark performance regressions. 🎉

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

JerAguilon pushed a commit to JerAguilon/arrow that referenced this pull request Oct 23, 2023
…#38176)

### Rationale for this change

It's an internal bundled library. We should not install it as a part of Arrow.

### What changes are included in this PR?

Exclude all Azure SDK for C++ jobs including install jobs aren't executed by default. Building jobs are executed because they are required to build Arrow.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: apache#37510

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…#38176)

### Rationale for this change

It's an internal bundled library. We should not install it as a part of Arrow.

### What changes are included in this PR?

Exclude all Azure SDK for C++ jobs including install jobs aren't executed by default. Building jobs are executed because they are required to build Arrow.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: apache#37510

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…#38176)

### Rationale for this change

It's an internal bundled library. We should not install it as a part of Arrow.

### What changes are included in this PR?

Exclude all Azure SDK for C++ jobs including install jobs aren't executed by default. Building jobs are executed because they are required to build Arrow.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: apache#37510

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C++] Bundled Azure SDK for C++ should not be installed
3 participants