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

bindings: Remove relative header paths #469

Merged
merged 3 commits into from
Mar 23, 2020
Merged

bindings: Remove relative header paths #469

merged 3 commits into from
Mar 23, 2020

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Mar 19, 2020

I've been having severe trouble building the bindings lately, because I tend to build in buildroots within the repo dir, and sometimes that repo dir is a git worktree inside my main clone of the repo.

So, there are plenty of times where ../../../include/Frame.h is a perfectly valid path from, say, the buildroot or the current build dir — but it's not the RIGHT Frame.h, and the build fails.

Thing is, at least on my system, those relative paths aren't needed. SWIG is (now) being passed the correct set of include prefixes so that it includes the correct include/ directory at the project root, so we can just use:

%include "Frame.h"
%include "Qt/PlayerPrivate.h"

etc.

I've set this up as a project repo branch, to see if the builders like it. If they don't, I'm confident I can convince them to see things my way.

@ferdnyc ferdnyc added code Source code cleanup, streamlining, or style tweaks build Issues related to compiling or installing libopenshot and its dependencies bindings libopenshot's Python or Ruby interface bindings labels Mar 19, 2020
@codecov-io
Copy link

Codecov Report

Merging #469 into develop will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #469   +/-   ##
========================================
  Coverage    48.12%   48.12%           
========================================
  Files          128      128           
  Lines         9934     9934           
========================================
  Hits          4781     4781           
  Misses        5153     5153           

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 2c6d25e...7403a9c. Read the comment docs.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Mar 20, 2020

Took a second or three, as I had to account for older CMake releases from before the big UseSWIG.cmake rewrite for 3.12, but I ultimately nailed it down.

Now the question becomes, do I do the same to the source files? I've got some large ideas, and will go write them up in a repo Issue to invite wider comment. (Issue link will be forthcoming.)

Use the include paths set on the command line the way they're
supposed to be used.
Fix SWIG include dirs
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Mar 23, 2020

Tried to be too clever, broke everything. Fixed now. 🤞

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Mar 23, 2020

Excellent, everything wound up where it belongs, on all four platforms. We can even build Ruby bindings now, without the install breaking on bad permissions! Woo.

@ferdnyc ferdnyc merged commit 94e9ad3 into develop Mar 23, 2020
@ferdnyc ferdnyc deleted the its-not-relative branch March 23, 2020 12:13
@SuslikV
Copy link
Contributor

SuslikV commented Mar 28, 2020

Too bad that you excluded this check from Windows:

        ### Calculate the python module path (using distutils)
        execute_process ( COMMAND ${PYTHON_EXECUTABLE} -c "\
...

and never added it back.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Mar 28, 2020

That was very ntentional, the Windows and Mac builds were installing into Python paths that had no relationship to reality, as well as no real usefulness.

Heck, the builders were manually MOVING the files up to a python/ dir under the install root, after make install finished. So, now they just get installed there directly.

Eventually we'll have the build packaging up the Python module properly and generating metadata, then pip can be used to install it wherever's needed. (Though on Windows, that's still not going to be any real use unless we include DLLs for libopenshot and all its dependencies in the package, too.)

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Mar 28, 2020

Now that a PYTHON_MODULE_PATH cache variable is respected by the build, you can still give it the old nonsense path on the cmake command line if necessary.

@SuslikV
Copy link
Contributor

SuslikV commented Apr 2, 2020

If I did build it with this "nonsense path" on Windows, then you just broke my set up by modifying the code only for linux ^_^ , so I've been need to update the paths variables to make it work again.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Apr 3, 2020

@SuslikV Well, I don't know how your Windows build is set up, but yes — I contend that whatever directory it tries to install to — I think it's usr\lib\python3.7\site-packages\ inside the install directory? — is a useless path, when it comes to building the Python module on Windows. I would think that, wherever it ends up installed, it's going to end up getting copied somewhere else.

Now, scripts that copy the module from that path to somewhere else, and expected it to be there, would have to be updated with the new path. I certainly had to change .gitlab-ci.yml in the project repo, to use the new path.

But that's why I announced the change in #458, and left it up for two weeks before merging.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Apr 3, 2020

But that's why I announced the change in #458, and left it up for two weeks before merging.

(The changes were also present in this PR, because I had to incorporate #458 to get these to work. But the install path change was originally merged in PR #458 which was submitted for that purpose.)

#458 also added the new support for custom paths specified in the cmake command via the -DPYTHON_MODULE_PATH and -DRUBY_MODULE_PATH variables. I was most assuredly not ignoring Mac or Windows builds, with that change — I was focusing on them, by exempting them from the Linux foolishness that's really only there for Debian and other distro packaging.

Python modules shouldn't be installed by CMake, anyway. They should be wrapped with appropriate metadata and installed by the proper ecosystem tools: setuptools and/or pip. But when you're talking about a module that imports a DLL, which itself has a long list of additional DLL dependencies, I can't imagine installing openshot.py and _openshot.dll in the systemwide site-packages directory using the Python packaging tools would be very useful, either.

@SuslikV
Copy link
Contributor

SuslikV commented Apr 3, 2020

Well, I don't know how your Windows build is set up ...

Hell starts here: https://github.com/SuslikV/openshot-qt/commits/patch-1 (CI folder) ^_^ Simple change, but as soon as destination was changed (only for Windows!) it was a little surprise for me. Python (and freeze) can find everything it needs if right path is known (so when first time I made libopenshot install I just added lib\site-packages to the python, so it know where to look for).

Nothing special about PR itself of new variables. Just about the way the changes are made (different behavior for platforms without real meaning but organizing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bindings libopenshot's Python or Ruby interface bindings build Issues related to compiling or installing libopenshot and its dependencies code Source code cleanup, streamlining, or style tweaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants