Skip to content

Conversation

@dae
Copy link
Contributor

@dae dae commented Oct 12, 2021

The setting was previously indiscriminately ignored, meaning it wasn't
possible to prevent third-party extension modules from being included
in the build.

Related: #405

…modules

The setting was previously indiscriminately ignored, meaning it wasn't
possible to prevent third-party extension modules from being included
in the build.

Related: indygreg#405
Copy link
Owner

@indygreg indygreg left a comment

Choose a reason for hiding this comment

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

Thanks for tracking this down.

I agree a partial solution here is better than none.

I'm going to rebase this and amend it slightly before pushing. I'll likely also tack on some tests in a follow-up commit because this behavior warrants testing!

@indygreg
Copy link
Owner

As I was implementing tests, I found a few issues with how extension modules are handled. I'd like to get the tests shored up before landing this so I can be confident of what the behavior is. I may not get this landed tonight. But it is on my short list.

indygreg added a commit that referenced this pull request Oct 20, 2021
PythonExtensionModule.add_include is not respected. See #456.
This commit adds a test for it.
indygreg added a commit that referenced this pull request Oct 20, 2021
PythonExtensionModule.add_include is not respected. See #456. And
the policy callbacks may not be fired for stdlib extension modules.

This commit adds some tests so we have better awareness of behavior
for when it inevitably changes.
@indygreg
Copy link
Owner

OK. So I've changed the API for the resource collector to return objects describing what actions were taken. I did this because as part of implementing tests for this, I encountered some weird behavior and found the existing logging around resource actions... lacking. So the new API enables us to print more useful log messages about what's going on.

This unfortunately introduced a merge conflict. The return Ok(None); in your PR should be changed to

return Ok((
    vec![AddResourceAction::NoInclude(extension_module.description())],
    None,
));

However, when I did this locally, a bunch of tests unrelated to the new ones started failing! I suspect the issue here has something to do with the original comment mentioning built-in extension modules and the new code only filtering on stdlib. The built-in set and the stdlib set aren't exactly the same and I think that is causing tests to fail.

This is a thorny situation. Having looked at this code, the tests I wrote, and the new log output (which appears to show duplicate extension adds), something is clearly off somewhere. I'll try to take a look at this again this week.

indygreg added a commit that referenced this pull request Oct 20, 2021
PythonExtensionModule.add_include is not respected. See #456. And
the policy callbacks may not be fired for stdlib extension modules.

This commit adds some tests so we have better awareness of behavior
for when it inevitably changes.
@dae
Copy link
Contributor Author

dae commented Oct 20, 2021

No rush - presumably it could be worked around by users for now by installing into a venv and pruning the unwanted packages before running pyoxidizer.

@indygreg indygreg force-pushed the main branch 3 times, most recently from 8f998b7 to 5c5ac33 Compare December 1, 2021 03:16
@indygreg indygreg force-pushed the main branch 2 times, most recently from 5e0524b to 789c62e Compare May 31, 2022 04:07
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