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

Don't fetch submodules for plugins that aren't being built #5182

Merged
merged 19 commits into from Oct 3, 2019

Conversation

tresf
Copy link
Member

@tresf tresf commented Sep 10, 2019

Quoting the bug report:

Currently, there is no way to specify which submodules you really want to "fetch", cmake will just init and update all. There are currently LMMS_MINIMAL and PLUGIN_LIST, but those are only used for compiling, not for git.

I think PLUGIN_LIST should be used by the in cmake/modules/CheckSubmodules.cmake

Expected advantages of this are:

  • reduction of compile time
  • reduction of download time and bandwidth
  • independence of broken submodules

Closes #5105

Per JENKINS-43977

"Just in case this helps someone else. The url in .gitmodules is case sensitive for the plugin. Running git in a command line is not case sensitive. This caused me some confusion since the git submodule update --init –recursive command would work from a command line, but I would get the following exception from the plugin. Once I changed the case in .gitmodules to match the case in the git repo the plugin worked."
This reverts commit c7c4dd6.

Lukas' name wasn't the problem.  The clone depth is.
@tresf
Copy link
Member Author

tresf commented Sep 13, 2019

@JohannesLorenz found a bug with --depth 100. Apparently, if our submodules are too outdated, it can't find the commit hash. We had some semi-working code for this by removing depth on failure, but bugs:

  • It never fell into that logic block
  • The submodule caches the depth value
  • Attempts to remove the cached value (including --no-recommend-shallow) are being ignored, perhaps because they're nested.

So the hack for now is to force a depth that's so high, the commit's guaranteed to be there. 🤷‍♂

This latest version will fallback on a full clone when the commits missing.

@tresf
Copy link
Member Author

tresf commented Sep 13, 2019

... sorry for so many pushes, the symptoms are unique to a fresh clone, so it requires a lot of trail and error. Edit: I think it's good now.

@JohannesLorenz
Copy link
Contributor

First of all, many thanks for this PR and your efforts with fixing the submodule issue!

Normally, I'd say many of those submodule related commits should go on top of master, since they are more urgent. But let's try to quickly review everything on this branch.

@tresf
Copy link
Member Author

tresf commented Sep 14, 2019

Normally, I'd say many of those submodule related commits should go on top of master, since they are more urgent. But let's try to quickly review everything on this branch.

I completely agree. I can separate the PRs if you wish. I was trying to fix testing while also drifting off course. :D

@JohannesLorenz
Copy link
Contributor

A few comments from testing:

  • The --depth fallback seems to work. I'd prefer the errors ("error: Server does not allow request ...") to not appear on screen if that would be possible, as I normally scan the output for errors, and the mass of errors on the first attempt can hide "real errors". This is just a nice-to-have though.
  • If you do something like -DSKIP_SUBMODULES=xxx (with xxx not existing), I'd vote for aborting without fetching any submodule. I.e. check if all are valid before doing anything. Imagine you have a type and this leeds to fetching the submodule, then you'll be busy getting rid of it again.
  • Same goes for PLUGIN_LIST if it contains rubbish: I think it should abort then without doing anything.
  • If you tell it to just skip exprtk (cmake -DSKIP_SUBMODULES=plugins/Xpressive/exprtk), it still complains it's missing exprtk/exprtk.hpp, see below.
  • Is there an easy way for a user how to find out what they can specify for -DSKIP_SUBMODULES or -DPLUGIN_LIST?
  • Can you please add a CMake option to force "full clones" for all submodules at first attempt?
CMake Error at cmake/modules/BuildPlugin.cmake:57 (ADD_LIBRARY):
  Cannot find source file:

    exprtk/exprtk.hpp

  Tried extensions .c .C .c++ .cc .cpp .cxx .cu .m .M .mm .h .hh .h++ .hm
  .hpp .hxx .in .txx
Call Stack (most recent call first):
  plugins/Xpressive/CMakeLists.txt:19 (BUILD_PLUGIN)


CMake Error at cmake/modules/BuildPlugin.cmake:57 (ADD_LIBRARY):
  No SOURCES given to target: xpressive
Call Stack (most recent call first):
  plugins/Xpressive/CMakeLists.txt:19 (BUILD_PLUGIN)

@tresf
Copy link
Member Author

tresf commented Sep 14, 2019

If you tell it to just skip exprtk (cmake -DSKIP_SUBMODULES=plugins/Xpressive/exprtk), it still complains it's missing exprtk/exprtk.hpp, see below.

So you want a reverse lookup too? That's quite a bit more work -- and I'd argue, one without a valid use-case.

Here's the original description:

Currently, there is no way to specify which submodules you really want to "fetch", cmake will just init and update all. There are currently LMMS_MINIMAL and PLUGIN_LIST, but those are only used for compiling, not for git.

This PR allows the git stuff to use them too, so it fulfills the requirement.

If you do something like -DSKIP_SUBMODULES=xxx (with xxx not existing), I'd vote for aborting without fetching any submodule. I.e. check if all are valid before doing anything. Imagine you have a type and this leeds to fetching the submodule, then you'll be busy getting rid of it again.

I strongly feel this is out of scope, lacks use-cases (the plugins remove the need for this now) and adds the potential for more bugs and delays to the PR, but I'll see if we can exit early in this scenario without causing regressions.

Same goes for PLUGIN_LIST if it contains rubbish: I think it should abort then without doing anything.

I didn't invent PLUGIN_LIST in this PR (instead, I'm simply honoring it), so I find this request misguided. Since we're already moving the PLUGIN_LIST logic, I'll take a look to see if it can be handled easily as part of this PR but it adds the potential for more bugs and delays to this PR.

Is there an easy way for a user how to find out what they can specify for -DSKIP_SUBMODULES

No.

or -DPLUGIN_LIST

I feel this is misguided. I didn't invent PLUGIN_LIST for this PR. Any communication to the user is out of scope. I'm just honoring it.

Can you please add a CMake option to force "full clones" for all submodules at first attempt?

I believe -DCLONE_DEPTH can do it, but it will need to be switched to a cached variable so that it doesn't overwrite on initialization. We'd also have to agree on some type of value, such as -DCLONE_DEPTH=False or-DCLONE_DEPTH=full.

@JohannesLorenz
Copy link
Contributor

If you tell it to just skip exprtk (cmake -DSKIP_SUBMODULES=plugins/Xpressive/exprtk), it still complains it's missing exprtk/exprtk.hpp, see below.

So you want a reverse lookup too? That's quite a bit more work -- and I'd argue, one without a valid use-case.

But what's the use of -DSKIP_SUBMODULES then, if I can't compile the code afterwards? I would need to exclude exprtk in PLUGIN_LIST to make it compile, but then, this would skip the submodule anyways (by your PR).

Is there an easy way for a user how to find out what they can specify for -DSKIP_SUBMODULES

No.

OK, in this case, a git submodule status should suffice

or -DPLUGIN_LIST

I feel this is misguided. I didn't invent PLUGIN_LIST for this PR. Any communication to the user is out of scope. I'm just honoring it.

Agreed, it can't be part of this PR.

Can you please add a CMake option to force "full clones" for all submodules at first attempt?

I believe -DCLONE_DEPTH can do it, but it will need to be switched to a cached variable so that it doesn't overwrite on initialization. We'd also have to agree on some type of value, such as -DCLONE_DEPTH=False or-DCLONE_DEPTH=full.

CMake uses ON and OFF for booleans. So maybe -DFULL_CLONES=ON|OFF?

- Document usage: submodues, plugins
- List available values: submodules, plugins
- Abort on bad values: submodules, plugins
@tresf
Copy link
Member Author

tresf commented Sep 14, 2019

CMake uses ON and OFF for booleans.

Well, it uses truth(y) logic, so that's the same as "True", but that's moot. I haven't added this yet, still on the list tho.

Agreed, it can't be part of this PR.

Haha, well it is now... added via 5df372f. I guess if we're going to make PLUGIN_LIST a bit more useful, it may be a good time to document it? To do this, I had to make an immutable list in cmake. I just copy it at build time. Hopefully this doesn't break anything. Testing greatly appreciated by people that use this feature.

what they can specify for -DSKIP_SUBMODULES

Try -DLIST_SUBMODULES=True per 5df372f.

[what they can specify for] -DPLUGIN_LIST

Try -DLIST_PLUGINS=True per 5df372f.

Furthemore if something's provided and it's wrong, it aborts with a helpful message. Any and all feedback appreciated.

@JohannesLorenz
Copy link
Contributor

Thanks for adding all that stuff 😃

Now, one issue I have is with cmake -DSKIP_SUBMODULES='zynaddsubfx;plugins/Xpressive/exprtk' .. : It passes the submodule check and then fails:

CMake Error at plugins/zynaddsubfx/CMakeLists.txt:69 (add_subdirectory):
  add_subdirectory given source "zynaddsubfx/src/Nio" which is not an
  existing directory.

Any ideas?

@JohannesLorenz
Copy link
Contributor

Would you find it practical to already show all submodules when one is wrong, instead of advising to type -DLIST_SUBMODULES=True?

@tresf
Copy link
Member Author

tresf commented Sep 15, 2019

Now, one issue I have is with cmake -DSKIP_SUBMODULES='zynaddsubfx;plugins/Xpressive/exprtk' .. : It passes the submodule check and then fails:

Whoops, looks like something is wrong? If you find it, feel free to patch it. Otherwise, I'll take a look ASAP.

Would you find it practical to already show all submodules when one is wrong, instead of advising to type -DLIST_SUBMODULES=True?

Different CLI apps treat this differently and it sort of makes sense why. Let's say we keep this moving forward, we'd have to be consistent and do the same for the plugins. Well, the plugin list is so large, is spams the error right off the screen. For this reason, some CLI apps echo correct values, others do not.

I'm fine either way as long as we're consistent.

@tresf
Copy link
Member Author

tresf commented Sep 17, 2019

Now, one issue I have is with cmake -DSKIP_SUBMODULES='zynaddsubfx;plugins/Xpressive/exprtk' .. : It passes the submodule check and then fails:

I tested and zyn's matching, so I think it's working as indicated. I had no intentions on using this as a reverse-lookup

@tresf
Copy link
Member Author

tresf commented Sep 17, 2019

Can you please add a CMake option to force "full clones" for all submodules at first attempt?

Added OPTION via -DNO_SHALLOW_CLONE=True

OPTION(NO_SHALLOW_CLONE "Disable shallow cloning of submodules" OFF)

@JohannesLorenz
Copy link
Contributor

Now, one issue I have is with cmake -DSKIP_SUBMODULES='zynaddsubfx;plugins/Xpressive/exprtk' .. : It passes the submodule check and then fails:

I tested and zyn's matching, so I think it's working as indicated. I had no intentions on using this as a reverse-lookup

What do you mean by "zyn's matching"? "zynaddsubfx" is no valid submodule (plugins/zynaddsubfx/zynaddsubfx is), so I'd expect it simply to abort.

@tresf
Copy link
Member Author

tresf commented Sep 17, 2019

The code is:

IF(${SUBMODULE_PATH} MATCHES ${_skip})
MESSAGE("-- Skipping ${SUBMODULE_PATH} matches \"${_skip}\" ${SKIP_REASON}")

Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a review. Some things to be done:

  • Resolve the comments if they make sense
  • More testing (I'll do this the next days)
  • While it was easy to code-read this, I'm not sure about the merge strategy.
    • We could easily squash all into one commit, which has both the bug fixes for lukas' name/full clone retry and the feature in only one commit. Would look unclean IMO.
    • We could do a simple merge.
    • You could squash/reorder the commits so they are only three (2 fixes, 1 feature) and I could rebase. Would that be OK?

cmake/modules/CheckSubmodules.cmake Outdated Show resolved Hide resolved
cmake/modules/CheckSubmodules.cmake Outdated Show resolved Hide resolved
cmake/modules/CheckSubmodules.cmake Show resolved Hide resolved
cmake/modules/CheckSubmodules.cmake Outdated Show resolved Hide resolved
@tresf
Copy link
Member Author

tresf commented Sep 20, 2019

Resolve the comments if they make sense

I'm only addressing one (the ugly LIST reuse). If you feel strongly about the rest, feel free to push them to this branch.

While it was easy to code-read this, I'm not sure about the merge strategy.
We could easily squash all into one commit, which has both the bug fixes for lukas' name/full clone retry and the feature in only one commit. Would look unclean IMO.

I'm fine squashing and not concerned about the readability or separation really. The --depth 100 could very well be its own bug report and PR, but sometimes these things get combined out of convenience to the developers. I'm fine with that.

I could rebase

I don't mind if you want to try. I'm not interested in doing it. The PR is representative of the work performed and we'll squash it out when it's ready for merge.

- Quote variables when needed
- Don't reuse a list for a separate purpose, it's confusing
- Process LIST_SUBMODULES before processing errors
@tresf
Copy link
Member Author

tresf commented Sep 20, 2019

All comments addressed, ready for review and testing. :D

@JohannesLorenz
Copy link
Contributor

cmake -DSKIP_SUBMODULES='plugins/Xpressive/exprtk;plugins/zynaddsubfx/zynaddsubfx' -DPLUGIN_LIST=LMMS_MINIMAL .. does not work either:

One or more submodule(s)
  "plugins/Xpressive/exprtk;plugins/zynaddsubfx/zynaddsubfx;plugins/zynaddsubfx/zynaddsubfx;plugins/FreeBoy/game-music-emu;plugins/OpulenZ/adplug;plugins/LadspaEffect/calf/veal;plugins/Xpressive/exprtk;plugins/LadspaEffect/swh/ladspa;plugins/LadspaEffect/tap/tap-plugins"
  was not found, aborting.

I guess it would work if you'd remove duplicates from the submodule list.

But before your fix it: What is the reason to expose SKIP_SUBMODULES to the user? Without reverse lookup, if a user uses SKIP_SUBMODULES, they are forced to also specify PLUGIN_LIST, which will supersede anything passed via SKIP_SUBMODULES.

@tresf
Copy link
Member Author

tresf commented Sep 28, 2019

But before your fix it: What is the reason to expose SKIP_SUBMODULES to the user? Without reverse lookup, if a user uses SKIP_SUBMODULES, they are forced to also specify PLUGIN_LIST, which will supersede anything passed via SKIP_SUBMODULES.

Love your point. I'd be happy to remove it. You're the first human to ever ask about it TBH.

@JohannesLorenz
Copy link
Contributor

OK, then let's hide it. It can be easily re-added later if there are complaints.

Also, consider updating your copyright notice.

This will reduce the amount for further tests significantly 😄

@JohannesLorenz
Copy link
Contributor

JohannesLorenz commented Sep 29, 2019

I wonder if we should call git submodule sync automatically, too. It's extremely fast and it's requires when a submodule remote URL changes. On the other hand, though, I'm not sure if there are cases where you explicitly want to not sync them. (This could be done in another PR though)

From a testing perspective, if you hide SKIP_SUBMODULES, I'm finished with testing. The only thing that I dislike is PLUGIN_LIST not stopping immediately if invalid plugin names are passed, but I'd like to try fixing this myself in a separate PR if you're OK with that.

@tresf
Copy link
Member Author

tresf commented Oct 3, 2019

Documentation for SKIP_SUBMODULES has been removed. The variable still exists and can technically be used, but aside from renaming it, we still need an internal way to track the submodules we've skipped, so I've kept it by variable name only.

@JohannesLorenz
Copy link
Contributor

Code/tests look good, I only found one issue: cmake -DPLUGIN_LIST=LMMS_MINIMAL .. results in:

CMake Error at plugins/CMakeLists.txt:19 (ADD_SUBDIRECTORY):
  ADD_SUBDIRECTORY given source "LMMS_MINIMAL" which is not an existing
  directory.

Can you reproduce/fix it?

@tresf
Copy link
Member Author

tresf commented Oct 3, 2019

Can you reproduce

Not on clone, but I didn't try to build.

Can you [...] fix it?

Probably. I took a second look at the code and my help text wrong. It's not -DPLUGIN_LIST=LMMS_MINIMAL, it's -DLMMS_MINIMAL=True but the help text doesn't say that.

I'll fix the help text now.

@JohannesLorenz
Copy link
Contributor

JohannesLorenz commented Oct 3, 2019

Indeed, -DLMMS_MINIMAL=True does work. 😄

I'll hit "squash and merge" as soon as you've pushed the help text fix (and the CI at least passed half-way).

@LmmsBot
Copy link

LmmsBot commented Oct 3, 2019

Downloads for this pull request

Generated by the LMMS pull requests bot.
SHA: cea4b50

@JohannesLorenz
Copy link
Contributor

One last thing, let's remove the LIST_SUBMODULES option. It's no where used anymore, right?

I'm writing the commit message meanwhile...

@JohannesLorenz JohannesLorenz merged commit 15fe551 into LMMS:master Oct 3, 2019
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
…hallow fallback (hotfix)

* Retry updating submodules non-shallow if shallow clone fails (master hotfix)
* Add `PLUGIN_LIST` support to CheckSubmodules (LMMS#5105)
* Remove `SKIP_SUBMODULES` switch (it's redundant to specifying `PLUGIN_LIST`)
* Add `NO_SHALLOW_CLONE` switch
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.

Add CMake switch to specify submodules
3 participants