◐ Shell
reader mode source ↗
Skip to content

Provide more info about failuers to load CLR assemblies#1076

Merged
lostmsu merged 1 commit into
pythonnet:masterfrom
losttech:PR/ImportErrorImprovement
Aug 19, 2020
Merged

Provide more info about failuers to load CLR assemblies#1076
lostmsu merged 1 commit into
pythonnet:masterfrom
losttech:PR/ImportErrorImprovement

Conversation

@lostmsu

@lostmsu lostmsu commented Mar 4, 2020

Copy link
Copy Markdown
Member

What does this implement/fix? Explain your changes.

  1. When trying to implicitly load assemblies, and that fails not because an assembly is missing, but because loading failed for some reason, emit Python warning.
  2. When trying to import a module in our import hook, if the module name is an assembly name, and CLR fails to load it, and Python also fails to find a module with the same name, add the exceptions we got from both during the attempt into __cause__ attribute of the final ImportError.

BREAKING: clr.AddReference will now throw exceptions besides FileNotFoundException.

Additional: a few uses of BorrowedReference

Does this close any currently open issues?

This addresses #261
It is an alternative to #298

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example

@lostmsu lostmsu force-pushed the PR/ImportErrorImprovement branch from 645687e to 4e1ea84 Compare March 4, 2020 02:47
@lostmsu lostmsu mentioned this pull request Mar 4, 2020
Closed
@lostmsu lostmsu force-pushed the PR/ImportErrorImprovement branch 2 times, most recently from ccf5203 to e6610eb Compare April 27, 2020 21:35
@lostmsu lostmsu force-pushed the PR/ImportErrorImprovement branch from e6610eb to 46a51d4 Compare May 13, 2020 21:05
@codecov-io

codecov-io commented May 14, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1076 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1076   +/-   ##
=======================================
  Coverage   86.66%   86.66%           
=======================================
  Files           1        1           
  Lines         300      300           
=======================================
  Hits          260      260           
  Misses         40       40
Flag Coverage Δ
#setup_linux 65.33% <ø> (ø)
#setup_windows 72.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72fae73...46a51d4. Read the comment docs.

@lostmsu lostmsu added this to the 3.0.0 milestone May 14, 2020
@filmor

filmor commented Aug 15, 2020

Copy link
Copy Markdown
Member

Fine by me, but please add a changelog entry for the breaking change.

@lostmsu lostmsu force-pushed the PR/ImportErrorImprovement branch 4 times, most recently from 58ef45f to 37df647 Compare August 18, 2020 20:43
@lostmsu
do not eat all exceptions when trying to add CLR reference, provide m… …
6eb9258 
…ore info when CLR assemblies are failed to be loaded

1. When trying to implicitly load assemblies, and that fails NOT because an assembly is missing, but because loading failed for some reason, emit Python warning.
2. When trying to import a module in our import hook, if the module name is an assembly name, and we fail to load it, and Python also fails to find a module with the same name, add the exceptions we got during the attempt to load it into __cause__ of the final ImportError

BREAKING: clr.AddReference will now throw exceptions besides FileNotFoundException.

Additional: a few uses of BorrowedReference

This addresses pythonnet#261
It is an alternative to pythonnet#298
@lostmsu lostmsu force-pushed the PR/ImportErrorImprovement branch from 37df647 to 6eb9258 Compare August 18, 2020 21:14
@lostmsu

lostmsu commented Aug 18, 2020

Copy link
Copy Markdown
Member Author

I dropped the update, that tried to remove legacy "CLR." module from this change, as it started growing in scope, and is not directly related to this change.

@codecov-commenter

codecov-commenter commented Aug 18, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1076 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1076   +/-   ##
=======================================
  Coverage   86.25%   86.25%           
=======================================
  Files           1        1           
  Lines         291      291           
=======================================
  Hits          251      251           
  Misses         40       40
Flag Coverage Δ
#setup_linux 64.94% <ø> (ø)
#setup_windows 72.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e1fc2e...6eb9258. Read the comment docs.

@lostmsu lostmsu merged commit 5b989cb into pythonnet:master Aug 19, 2020
@lostmsu lostmsu deleted the PR/ImportErrorImprovement branch August 19, 2020 06:29
@filmor filmor mentioned this pull request Feb 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@filmor filmor filmor approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@lostmsu @codecov-io @filmor @codecov-commenter