Skip to content

Conversation

@hmeriann
Copy link
Contributor

@hmeriann hmeriann commented Jul 12, 2024

This PR removes excluding architectures as discussed here and put repeating job into a matrix

@hmeriann
Copy link
Contributor Author

@carlopi could you please take a look at the changes I've made here?

Copy link
Contributor

@carlopi carlopi left a comment

Choose a reason for hiding this comment

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

Thanks, makes a lot of sense.

I think this was originally made to allow this by @samansmink, pinging him since he might be curious.

PR is good to go!

@samansmink
Copy link

Is there a specific reason you want to build sqlsmith against both main and v1.0.0? I know @Maxxen does it too for spatial ,but I think it might be a bit much to run both always

@hmeriann
Copy link
Contributor Author

PR is good to go!

Thanks, @carlopi !

I'm not sure I have a good answer to @samansmink 's question. I guess we want to build sqlsmith against both main and v1.0.0 to see, if it's compatible with current main version, not only with the last stable. But should we?

@Tmonster , should we build sqlsmith against both (main and v1.0.0)? How to decide?

@samansmink
Copy link

@hmeriann there's nothing fundamentally wrong, but it might be wasteful. Note that DuckDB CI also builds the sqlsmith extension on every PR at the commit specified here https://github.com/duckdb/duckdb/blob/18c59ed4b42d1ef4692973f82361d6bce2af7092/.github/config/out_of_tree_extensions.cmake#L104

In general I would say the most natural process for extensions is to:

  • build against latest stable DuckDB untill either:
    • the extension requires a feature from DuckDB main
    • a new DuckDB release is happening soonish

However, for sqlsmith this might be different as this extension is mostly used in CI anyway so perhaps targeting main always makes sense here? Note that targeting main can be a little unstable as that means that the sqlsmith ci can suddenly break due to changes in duckdb/duckdb.

@hmeriann
Copy link
Contributor Author

Thanks @samansmink for the clarifications! Building sqlsmith only against current main makes more sense then

@hmeriann
Copy link
Contributor Author

Closing this since it's already been made in this PR #11

@hmeriann hmeriann closed this Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants