Skip to content

Conversation

@mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Mar 19, 2022

Adding pyproject.toml

Fixes #152

@CLAassistant
Copy link

CLAassistant commented Mar 19, 2022

CLA assistant check
All committers have signed the CLA.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 20, 2022

This doesn't work

    error: file '/Users/mkoeppe/s/sage/CyLP/cylp/cy/CyTest.pxd' does not exist
    [end of output]

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 20, 2022

Put them into extra_files instead.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 20, 2022

By the way, why does it install the cpp files like ICoinPackedMatrix.cpp? Should I remove this?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 20, 2022

I think the whole customInstall can go too. This whole isspace business is probably outdated by years

@tkralphs
Copy link
Member

tkralphs commented Mar 20, 2022

Hmm, that is strange. I was able to build successfully last night, but building b0cc169 just now, I got the same error as you about non-existence of CyTest.pxd (which actually doesn't exist, makes sense). But then I don't know why everything worked for you. Anyway... I removed the .pxd files from sources (you are right that they should be treated as headers) and also got rid of the custom install stuff, which you are right is probably not needed (I do recall that it was at one time). Now everything is again working for me and simplified from what it was.

As for the cpp files like ICoinPackedMatrix.cpp, those are definitely needed for the build and are not auto-generated so I don't think we can remove them. I have struggled to understand how this whole thing hangs together myself, but those I*.cpp files are extensions (derived classes, actually) of the Cbc classes that add some functions needed by CyLP.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 20, 2022

As for the cpp files like ICoinPackedMatrix.cpp, those are definitely needed for the build and are not auto-generated so I don't think we can remove them.

Yes, they are needed at build time. They are compiled and linked into the extension modules.
My question was whether these source files need to be installed.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 20, 2022

In other words, I'm proposing this:

diff --git a/setup.py b/setup.py
index 646b024..5e70cfe 100644
--- a/setup.py
+++ b/setup.py
@@ -396,7 +396,7 @@ ext_modules += [Extension('cylp.cy.CyCutGeneratorPythonBase',
 s_README = getBdistFriendlyString(myopen('README.rst').read())
 s_AUTHORS = u(open('AUTHORS').read())
 
-extra_files = ['cpp/*.cpp', 'cpp/*.hpp', 'cpp/*.h', 'cy/*.pxd', 'VERSION']
+extra_files = ['cpp/*.hpp', 'cpp/*.h', 'cy/*.pxd', 'VERSION']
 
 setup(name='cylp',
       version=VERSION,

@tkralphs
Copy link
Member

OK, sorry, when you said "should I remove these," I thought you meant from the repo, not just from the list of files to be installed. It seems they don't need to be installed, other than just for the purpose of including all the source files in the distribution. I'll do as you suggested.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 20, 2022

Great. I think this PR is ready now.

@tkralphs
Copy link
Member

OK, I tested in Windows also and was able to build a wheel there, so I'll go ahead and merge this.

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.

sdist should ship Cython sources

3 participants