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

Bisecting between 0.9.2 and 0.10.0 #96

Closed
wants to merge 55 commits into from
Closed

Conversation

xhochy
Copy link
Member

@xhochy xhochy commented Feb 21, 2024

WIP

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@xhochy
Copy link
Member Author

xhochy commented Feb 21, 2024

The last run was actually ✅

@xhochy
Copy link
Member Author

xhochy commented Feb 21, 2024

The last run is broken in a different way.

@xhochy
Copy link
Member Author

xhochy commented Feb 21, 2024

Same broken commit again.

@xhochy
Copy link
Member Author

xhochy commented Feb 22, 2024

Unrelated error.

@xhochy
Copy link
Member Author

xhochy commented Feb 22, 2024

Different error:

2024-02-22T09:01:32.1390896Z      Creating library build\temp.win-amd64-cpython-311\Release\duckdb\extension\fts\duckdb.cp311-win_amd64.lib and object build\temp.win-amd64-cpython-311\Release\duckdb\extension\fts\duckdb.cp311-win_amd64.exp
2024-02-22T09:01:32.1832502Z   mbedtls_wrapper.obj : error LNK2001: unresolved external symbol mbedtls_gcm_init
2024-02-22T09:01:32.4703712Z   mbedtls_wrapper.obj : error LNK2001: unresolved external symbol mbedtls_gcm_free
2024-02-22T09:01:32.4705243Z   mbedtls_wrapper.obj : error LNK2001: unresolved external symbol mbedtls_gcm_starts
2024-02-22T09:01:32.4713650Z   mbedtls_wrapper.obj : error LNK2001: unresolved external symbol mbedtls_gcm_update
2024-02-22T09:01:32.4714964Z   mbedtls_wrapper.obj : error LNK2001: unresolved external symbol mbedtls_gcm_setkey

Skipping this commit for now.

@xhochy
Copy link
Member Author

xhochy commented Feb 22, 2024

Different error again:

2024-02-22T10:10:12.2370730Z   duckdb_build/extension/parquet/parquet_writer.cpp:456:3: error: use of undeclared identifier 'asdas'
2024-02-22T10:10:12.2472450Z                   asdas
2024-02-22T10:10:12.2573170Z                   ^
2024-02-22T10:10:15.5954910Z   1 error generated.

Skipping the commit.

@xhochy
Copy link
Member Author

xhochy commented Feb 26, 2024

Next split is at duckdb/duckdb@c2ef0bc

@xhochy
Copy link
Member Author

xhochy commented Feb 26, 2024

Checking the next split at duckdb/duckdb@0cb071a

@xhochy
Copy link
Member Author

xhochy commented Feb 28, 2024

Started a new bisect between 0afce1eb36fc5338bb0a2a65a893a96dc65e0139 and 0.9.2. Somewhere I have skipped too much.

@xhochy
Copy link
Member Author

xhochy commented Mar 1, 2024

@Mytherin This is what I come down to at the end of bisecting. I thought that the first time, I skipped too much but it seems like this is really the list of possible failing commits:

% git bisect skip
There are only 'skip'ped commits left to test.
The first bad commit could be any of:
105b0a4af57d2641ae7df9b08c63ec92f9c4902b
3308df3337acf5fde06423aadc9ee281aced334b
d3e7c4bb8675a25d9993fe8d9c61bce8f4d27f22
767a35aee5e90c7c68e213900cda765a3f9ff582
b972ff5d5fa64fce448537fb35b4a4c860cba5bd
097d7be5c113bddb11f909cbb02edc34b17767ad
b90072b1322e5eae145a9c9453c6a58624e4aabd
9c5ad19fb45d2fbf8dbc174c400b378d3b872724
b418d68d4e25615df9289095e07e8b2358198857
8ad5e6eb0a948815078cffcae631fa23bd850ea3
b9b895008f18a8a85e858bc034ff9d4db959375f
7114080b0bee8dff61de0d7c033d98ac1ef87e0f
d93ce2952729d0e0af7334d2e55d4800f8b00175
efa0da10756f18f32a98f0bf5599caae4ddd0089
f4a5919b956febf0f15e69c0c23536babae3129a
337f0f1bd3f5aa5e7798f5c013f127f63bd8ddb1
We cannot bisect more!

@Mytherin
Copy link

Mytherin commented Mar 1, 2024

Thanks for investigating! Those all seem to point towards the Parquet encryption support that we added - duckdb/duckdb#9392. There is actually also an open issue with Parquet encryption on Windows - duckdb/duckdb#10752 - although that is a run-time error not a compile time one. Nevertheless this seems like a very likely culprit.

@Mytherin
Copy link

Mytherin commented Mar 9, 2024

I have been investigating this locally on my Windows box and managed to reproduce the issue - and I think I figured out the problem. The total length of the lnk.exe command is now 34218 bytes, which exceeds the 32767 byte limit on Windows. The linked PR caused us to pass this threshold as it adds a number of new source files. In our own Windows Python CI we run with the DUCKDB_BUILD_UNITY=1 environment variable, and run on a bash shell. I think that either (1) DUCKDB_BUILD_UNITY causes fewer lines to be emitted as it structures the source files in a different manner, (2) using bash instead of cmd avoids running into the limit, or (3) perhaps a different setup means the prefix for each of the sources is smaller.

I will see if we can instead batch some of these source files into separate libraries so that we don't need to pass all of our source files to the linker all at once.

@Mytherin
Copy link

Mytherin commented Mar 9, 2024

See also this open issue in setuptools - pypa/setuptools#4177

@Mytherin
Copy link

Mytherin commented Mar 9, 2024

Ah - now I remember, there was this work-around that @Mause patched in here - duckdb/duckdb#10159

I suppose this is not working somehow in the Conda build.

@Mytherin
Copy link

Mytherin commented Mar 9, 2024

I have a fix up here that moves all source files into the root directory, which should reduce the paths below the threshold again - duckdb/duckdb#11068

@Mytherin
Copy link

Mytherin commented Mar 9, 2024

I think this should now be fixed in the latest main - would be great if you could re-run the CI and see if it works now

@xhochy
Copy link
Member Author

xhochy commented Mar 12, 2024

@Mytherin It works 🎉

@Mytherin
Copy link

Awesome! We are planning to release v0.10.1 next week which will ship with this fix

@xhochy xhochy closed this Mar 19, 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.

2 participants