Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented May 31, 2023

Which issue does this PR close?

Closes #6495

Rationale for this change

CI / builds are failing after the update of the ctor library

What changes are included in this PR?

Ensure we only use #[ctor] once in the datafusion-optimizer library

Are these changes tested?

yes by CI

Are there any user-facing changes?

@alamb alamb added the development-process Related to development process of DataFusion label May 31, 2023
@alamb alamb marked this pull request as ready for review May 31, 2023 17:44
@github-actions github-actions bot added optimizer Optimizer rules and removed development-process Related to development process of DataFusion labels May 31, 2023
};
use std::ops::Add;

#[cfg(test)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something about ctor 2.1 doesn't like two ctors for a single library

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

Not a massive fan of the use of static constructors, but as this is a pre-existing pattern 👍

@alamb
Copy link
Contributor Author

alamb commented May 31, 2023

Not a massive fan of the use of static constructors, but as this is a pre-existing pattern 👍

FWIW the only alternative I know is to do what we do in IOx and sprinkle setup in all the tests:

https://github.com/search?q=repo%3Ainfluxdata/influxdb_iox%20maybe_start_logging&type=code

e.g.

    #[test]
    fn test_panic_counter_and_logging() {
        maybe_start_logging();

Do you know a better pattern to enable logging during unit test run?

@avantgardnerio
Copy link
Contributor

Not a massive fan of the use of static constructors, but as this is a pre-existing pattern +1

FWIW the only alternative I know is to do what we do in IOx and sprinkle setup in all the tests:

https://github.com/search?q=repo%3Ainfluxdata/influxdb_iox%20maybe_start_logging&type=code

e.g.

    #[test]
    fn test_panic_counter_and_logging() {
        maybe_start_logging();

Do you know a better pattern to enable logging during unit test run?

yes, one second

@avantgardnerio
Copy link
Contributor

avantgardnerio commented May 31, 2023

Not a massive fan of the use of static constructors, but as this is a pre-existing pattern +1

FWIW the only alternative I know is to do what we do in IOx and sprinkle setup in all the tests:

https://github.com/search?q=repo%3Ainfluxdata/influxdb_iox%20maybe_start_logging&type=code

e.g.

    #[test]
    fn test_panic_counter_and_logging() {
        maybe_start_logging();

Do you know a better pattern to enable logging during unit test run?

What about this? https://github.com/apache/arrow-datafusion/blob/201a2a903325ad4dddd94572cfd0a19287131f04/datafusion/sql/tests/integration_test.rs#LL41C7-L41C7

Oh, never mind, I see now the conversation started with not liking static constructor.

@alamb alamb merged commit 79c67c2 into apache:main May 31, 2023
@alamb alamb deleted the alamb/fix_build2 branch May 31, 2023 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Main builds fail with error: symbol init___rust_ctor___ctor is already defined

4 participants