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

update build-kdenlive.sh for Qt5/KF5 version #8

Merged
merged 1 commit into from
Dec 29, 2015

Conversation

vpinon
Copy link
Contributor

@vpinon vpinon commented Dec 11, 2015

Hello!
The build script hasn't been updated on your side to follow Kdenlive's switch to KF5;
Roger Morton did the base change, and then I played a bit to adapt the output to my distribution needs.
Interested to host the changes?
Thanks again and again for your work on MLT and all the rest :)
Vincent

@bmatherly
Copy link
Member

Hi Vincent,

Thanks for sharing. I have some questions:

  1. USE_KF5 doesn't seem to do anything except for testing for the presence of KF5. But it doesn't change any build parameters. What is the purpose of this? Can it be excluded?
  2. Why do you change the default parameters to use release branches? I would rather not chase release branch changes in the build script. Those parameters can be changed using build-kdenlive.conf. So I am not really interested in accepting the default parameter changes - unless there is a really good reason to change the default.
  3. Same comment for SOURCES_CLEAN, ENABLE_MOVIT, H264, LAME, MP3, INSTALL_DIR
  4. There are some ugly comments: "# vvvvv" "# v v v v v v v v v". I would like those to be excluded.
  5. Are the ARCHIVE and UPLOAD features used by the KDENLIVE build system? What is their purpose? I'm not sure if I want to include them in my script. Maybe if I understood them better.

I think that the patch is a mix of changes that are required and changes that are personal preference. Would it be possible to create a patch that only includes required changes?

Thanks,

~BM

@ddennedy
Copy link
Member

Some things to think about. After accepting it, then we will probably turn off kdenlive builds on our nightly build server; Kdenlive is using KDE CI now, which is fine. The changes should then be integrated for 2 reasons: because this has been historically the authoritative place to maintain the script and because some people want to run it (not just build servers). Thus, I think Brian's comments are relevant for this pull request.
Another option is to remove the script from here, Kdenlive put it in their repo to maintain it for their so-called bundle builds. Then, we direct people to that location from the MLT website, etc.

@vpinon
Copy link
Contributor Author

vpinon commented Dec 11, 2015

@bmatherly

You're right, I did mix in the same branch the fixes for KF5
and my preferences for building process & output libs inclusions => I will separate these

  1. USE_KF5 triggers "test_kf5_available" that dies if KF5 libs are not >= 5.9
  2. that was my preference for files I upload to files.kde.org...
    I was not aware of the conf file possibility!
  3. same, kde.org asked me to remove any uncertain lib; +personal build convenience
  4. that is ttguy's style of commenting, not of interest in this merge you're right
  5. As on my very weak machine the compilation takes ages, I fire the script and it creates archive and sends it on server, I can go to sleep while it works ;)

I will cleanup all this and come back to you (maybe not this evening)

Cheers,

Vincent.

@vpinon
Copy link
Contributor Author

vpinon commented Dec 11, 2015

@ddennedy
Yes the idea was to unload you from that task (many thanks for the service for so long, I believe the cost is not insignificant?)
KDE CI didn't prove useful for users as it doesn't output binaries; Kubuntu CI neither as it changes your whole system every day/week (and is ubuntu only)...
You're right that many people are running it; yes we could deliver it with the source or somewhere else on kdenlive.org/kde.org, I don't really have an opinion.

@ddennedy
Copy link
Member

Vincent, you recently made a KF5 Kdenlive app bundle using this revised script. Do you have a way to re-run this periodically to create app bundles for user download? If so, I think you should takeover hosting and maintenance of the script as it is not trivial for us to update our build nodes for KF5.

@vpinon
Copy link
Contributor Author

vpinon commented Dec 13, 2015 via email

@vpinon
Copy link
Contributor Author

vpinon commented Dec 14, 2015

Hello,
In case you still want to sync the KF5 update, I've reduced the diff, an put my customization in my own conf file.
Are you still interested, have comments?
BR,
Vincent.

@bmatherly
Copy link
Member

I am interested in the changes. Do you know if your new script works with KDE4?

I have a test machine set up that has KDE4 installed and does not have KDE5. When I set USE_KF5=0 and run the script, I get an error:

CMake Error at CMakeLists.txt:24 (find_package):
  Could not find a package configuration file provided by "ECM" (requested
  version 1.2.0) with any of the following names:

    ECMConfig.cmake
    ecm-config.cmake

  Add the installation prefix of "ECM" to CMAKE_PREFIX_PATH or set "ECM_DIR"
  to a directory containing one of the above files.  If "ECM" provides a
  separate development package or SDK, be sure it has been installed.

This particular machine is running Ubuntu 14.04 and they didn't offer a cmake-extra-modules package until 14.10. So I hesitate to install cmake-extra-modules manually. Besides, the old script works just fine on this same machine. So I'm not sure why the new script doesn't work.

@bmatherly
Copy link
Member

Oh, I see the issue.

The old script has:
KDENLIVE_REVISION=origin/v0.9.x
The new script has:
KDENLIVE_REVISION=
which would use master.

When I use the new script and set KDENLIVE_REVISION=origin/v0.9.x then everything works fine.

So I assume that master does not support KDE4. Makes sense.

bmatherly added a commit that referenced this pull request Dec 29, 2015
update build-kdenlive.sh for Qt5/KF5 version
@bmatherly bmatherly merged commit f5bf908 into mltframework:master Dec 29, 2015
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.

None yet

3 participants