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

(#22506) arrow: fix config_options #22508

Closed
wants to merge 2 commits into from
Closed

Conversation

wgtmac
Copy link
Contributor

@wgtmac wgtmac commented Jan 23, 2024

Specify library name and version: arrow/15.0.0

Fix #22506


@CLAassistant
Copy link

CLAassistant commented Jan 23, 2024

CLA assistant check
All committers have signed the CLA.

@wgtmac wgtmac changed the title arrow: fix config_options (#22506) arrow: fix config_options Jan 23, 2024
@wgtmac
Copy link
Contributor Author

wgtmac commented Jan 23, 2024

@kou @raulcd

@ericLemanissierBot
Copy link

ericLemanissierBot commented Jan 23, 2024

I detected other pull requests that are modifying arrow/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@conan-center-bot

This comment has been minimized.

@valgur
Copy link
Contributor

valgur commented Jan 23, 2024

See #21669.

Thank you for the PR, but this approach would break most of the handling of dependent option values.

@raulcd
Copy link
Contributor

raulcd commented Jan 23, 2024

We can try to apply: #21669 to our sync PR: apache/arrow#39729

@wgtmac
Copy link
Contributor Author

wgtmac commented Jan 24, 2024

@valgur I have applied #21669 to apache/arrow#39729 and it works. Could you help moving forward #21669? Then I can sync it to the arrow repo. Thanks!

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@wgtmac
Copy link
Contributor Author

wgtmac commented Feb 27, 2024

@valgur Do you mind taking a look at my proposed fix?

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 9 (d31ebc03df1ce16e5adb2a093ea02c2a73703f29):

  • arrow/15.0.0:
    All packages built successfully! (All logs)

  • arrow/14.0.2:
    All packages built successfully! (All logs)

  • arrow/13.0.0:
    All packages built successfully! (All logs)

  • arrow/14.0.0:
    All packages built successfully! (All logs)

  • arrow/8.0.1:
    All packages built successfully! (All logs)

  • arrow/14.0.1:
    All packages built successfully! (All logs)

  • arrow/11.0.0:
    All packages built successfully! (All logs)

  • arrow/12.0.0:
    All packages built successfully! (All logs)

  • arrow/10.0.1:
    All packages built successfully! (All logs)

  • arrow/8.0.0:
    All packages built successfully! (All logs)

  • arrow/2.0.0:
    All packages built successfully! (All logs)

  • arrow/10.0.0:
    All packages built successfully! (All logs)

  • arrow/7.0.0:
    All packages built successfully! (All logs)

  • arrow/12.0.1:
    All packages built successfully! (All logs)

  • arrow/1.0.0:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 8 (d31ebc03df1ce16e5adb2a093ea02c2a73703f29):

  • arrow/14.0.0:
    All packages built successfully! (All logs)

  • arrow/15.0.0:
    All packages built successfully! (All logs)

  • arrow/13.0.0:
    All packages built successfully! (All logs)

  • arrow/14.0.2:
    All packages built successfully! (All logs)

  • arrow/14.0.1:
    All packages built successfully! (All logs)

  • arrow/12.0.0:
    All packages built successfully! (All logs)

  • arrow/2.0.0:
    All packages built successfully! (All logs)

  • arrow/12.0.1:
    All packages built successfully! (All logs)

  • arrow/11.0.0:
    All packages built successfully! (All logs)

  • arrow/10.0.1:
    All packages built successfully! (All logs)

  • arrow/8.0.0:
    All packages built successfully! (All logs)

  • arrow/10.0.0:
    All packages built successfully! (All logs)

  • arrow/8.0.1:
    All packages built successfully! (All logs)

  • arrow/7.0.0:
    All packages built successfully! (All logs)

  • arrow/1.0.0:
    All packages built successfully! (All logs)

@wgtmac
Copy link
Contributor Author

wgtmac commented Feb 28, 2024

Also ping @toge @RubenRBS @uilianries for help.

To provide the context, this PR is based on #21669 (by @valgur) with a few lines to fix the simd_level option.

@wgtmac
Copy link
Contributor Author

wgtmac commented Mar 4, 2024

Thanks for the review! @valgur

@wgtmac
Copy link
Contributor Author

wgtmac commented Mar 4, 2024

@toge Could you help review this? It would be good to get this in before the Arrow community releasing 15.0.1. Thanks!

@raulcd
Copy link
Contributor

raulcd commented Mar 5, 2024

@RubenRBS @toge @uilianries could we get a review and a merge, I am about to release 15.0.1 and it would be good for us to get this merged before. Thank you very much!

@wgtmac
Copy link
Contributor Author

wgtmac commented Mar 7, 2024

Thanks @RubenRBS for the assignment!

Hi @danimtb, would you mind taking a look?

@raulcd
Copy link
Contributor

raulcd commented Mar 8, 2024

Hi, we have just released 15.0.1 but I am not going to create a PR on conan until this is merged as we should merge this first.

@wgtmac
Copy link
Contributor Author

wgtmac commented Mar 11, 2024

@RubenRBS Gentle ping. Could you help review this?

@danimtb
Copy link
Member

danimtb commented Mar 12, 2024

Hi @wgtmac and sorry for the delayed response.

This recipe has given us some trouble in the past with several PRs trying to improve the management of options. There are too many of them and some of them affect others or give them a spcific value.

We have discussed this internally in the conan team and we think that we don't want to introduce more code in this recipe con manage options. Instead, we would like to simplify the recipe and remove any kind of automatic management of options (as it has proved to be problematic) and just restrict the combinations in the validate() method and direct the user to input the correct value for each option.

We believe this would be the less problematic approach that hopefully would solve the issues of this recipe regarding options and its management.

Hope that it make sense. Thank you!

@wgtmac
Copy link
Contributor Author

wgtmac commented Mar 13, 2024

Thanks for the reply! @danimtb

Instead, we would like to simplify the recipe and remove any kind of automatic management of options (as it has proved to be problematic) and just restrict the combinations in the validate() method and direct the user to input the correct value for each option.

That sounds good to me. We could simply pass the Conan option to the corresponding CMake option and those CMake files in the Arrow repo have already handled the relationship of options. IMHO, the remaining challenge is to figure out the third-party dependency requirement based on the user options. Will someone from the Conan community work on this?

@danimtb
Copy link
Member

danimtb commented Mar 13, 2024

IMHO, the remaining challenge is to figure out the third-party dependency requirement based on the user options. Will someone from the Conan community work on this?

Yes, we will try to work on that!

@wgtmac
Copy link
Contributor Author

wgtmac commented Mar 13, 2024

That's awesome! Looking forward to the fix. @danimtb

@valgur
Copy link
Contributor

valgur commented Apr 9, 2024

Can be closed after #23163 was merged.

@danimtb danimtb closed this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[package] arrow/15.0.0: user options are not respected
7 participants