Skip to content

ABI.Initialize gives a helpful message when the TypeOffset interop class is not configured correctly #1340

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

Merged
merged 7 commits into from
Jan 4, 2021

Conversation

tminka
Copy link
Contributor

@tminka tminka commented Dec 30, 2020

What does this implement/fix? Explain your changes.

When changing Python versions, it is easy to put Python.NET into a misconfigured state. This PR provides a helpful error message in that case.

Does this close any currently open issues?

No

Any other comments?

This change can be tested by manually editing configured.props to have an incorrect PythonInteropFile

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
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@lostmsu
Copy link
Member

lostmsu commented Dec 31, 2020

I actually think the problem is this line should have been removed:

<Compile Remove="interop*.cs" />

Changing Python version would also mean NativeTypeOffset must be regenerated (if present), and currently I see no easy way to detect that scenario. Perhaps NativeTypeOffset should have version embedded and checked somewhere.

@filmor
Copy link
Member

filmor commented Dec 31, 2020

@lostmsu Well, that one doesn't do much, I don't see what that would bring. Adding version and ABI flags to the interop classes and comparing against those should be more stable.

@tminka The advice should be to run python setup.py configure.

Python.Runtime.csproj includes all interopXX.cs files
@tminka
Copy link
Contributor Author

tminka commented Dec 31, 2020

@filmor I have changed the advice as directed.

@lostmsu Your suggestion works. If Python.Runtime.csproj includes all interopXX.cs files, then an incorrect value of PythonInteropFile does not break the build. This seems like a useful robustness to have, so I added it.

@tminka
Copy link
Contributor Author

tminka commented Jan 1, 2021

The test failures imply that NativeTypeOffset is in fact not generated on some non-Windows platforms. I'm not sure what to do about that.

@lostmsu
Copy link
Member

lostmsu commented Jan 1, 2021

It seems to be a temporary fluke, the test failures are identical between this and your other PR, which are unrelated.

@lostmsu
Copy link
Member

lostmsu commented Jan 1, 2021

Oh @tminka , you meant this failure: f59e5d0

I think this is because you must keep

  <ItemGroup Condition=" '$(PythonInteropFile)' != '' ">	
    <Compile Include="$(PythonInteropFile)" />	
  </ItemGroup>

I believe the file is generated outside src\runtime folder, and thus is not included into default compile items.

@tminka
Copy link
Contributor Author

tminka commented Jan 3, 2021

I have restored the lines that you mentioned, and the same tests fail. So I don't think that was the problem. Also, if you look at the output of the "Build and Install" step, ubuntu 3.6 prints Creating src/runtime/interop36m.cs but ubuntu 3.8 and 3.9 do not. Therefore they are not generating any interop file. Also, if you read setup.py you will see that it always creates the interop file in src\runtime, so it is not possible for the file to be generated outside src\runtime folder.

Perhaps the reason that no interop file is generated is because sys.abiflags was changed in 3.8 to be an empty string. Indeed, MacOS does not generate an interop file for Python 3.8 either. The reason that the "MacOS, 3.8" build succeeds is that the embedded tests were skipped. Maybe "ubuntu, 3.8" should also skip the embedded tests.

@lostmsu lostmsu merged commit cac82a6 into pythonnet:master Jan 4, 2021
@tminka tminka deleted the initialize branch January 4, 2021 23:09
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