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

cmake: when not SC_QT skip install GUI help but not Guides #5042

Merged
merged 3 commits into from Jul 1, 2020

Conversation

sonoro1234
Copy link
Contributor

@sonoro1234 sonoro1234 commented Jun 29, 2020

Purpose and Motivation

When SC_QT is off cmake regex is skipping GUI in filename so that Guides are also skipped (issue #5043)

Types of changes

Change cmake regex to skip GUI followed by . or _ or - which are the patterns I could find in gui help files

@sonoro1234 sonoro1234 added comp: build CMake build system comp: help schelp documentation labels Jun 29, 2020
@sonoro1234 sonoro1234 requested a review from mossheim June 29, 2020 11:32
@mossheim mossheim removed their request for review June 29, 2020 23:03
@mossheim
Copy link
Contributor

IIUC, the original intention of this regex was to exclude specific directories of SCClassLibrary. my understanding seems to be supported by this history:

in 1e5135dac , which introduced this exclusion feature, it is clearly intended to exclude directories (.git and QtCollider).

061c5b50cc2 clearly treats this regex as applying to only directories (in the symlinking code)

#3456, which switched the regex from "QtCollider" to "GUI", also seemed to assume that this would only ever apply to folders.

using this regex when installing from HelpSource appears to be a mistake. it was originally done in this commit: d7b078c99

which appears to be an accidental copy-paste of the SCClassLibrary installation command. i say it is accidental because at that point in time the regex could only ever contain "QtCollider", but there was no folder or file name QtCollider in HelpSource at that time. probably it went unnoticed because of that.

there is also another aspect here which is that this regex appears to behave case-insensitively on your system (but not on mine). we should definitely support case insensitive filesystems by not behaving differently (i.e. installing or not installing different files and directories).

i believe the correct fix here is:

  • do not apply this regex to HelpSource (that fixes the problem you were intending to solve)
  • change the regexes to the more specific /GUI$ and /Ableton$ so that this indeed only applies to directories as everyone already appears to think it does
  • remove the regex check on line 484 of CMakeLists.txt, because it currently does nothing and only causes confusion (this will check only the immediate subdirectories of SCClassLibrary, none of which are named GUI or Ableton)
  • deal with one outlier in SCClassLibrary, UnitTestGUI.sc, by creating SCClassLibrary/Common/UnitTesting/GUI and moving UnitTestGUI.sc into it.
  • consider doing the same for SCClassLibrary/Platform/linux/LIDGui.sc and SCClassLibrary/Common/Quarks/QuarksGui.sc, as on a case-insensitive filesystem these are currently being excluded in a non-GUI install, and they are definitely GUI- and GUI-only code.

@sonoro1234
Copy link
Contributor Author

sonoro1234 commented Jun 30, 2020

@brianlheim Wow, You have done a throughout and deep investigation on this issue.
These are a lot of changes!!!. Do you want me to do all of them or perhaps I should close this PR and let you implement them?

May be another solution could be just dont change anything when SC_QT is off?
Would it be harmful to have help and Classes related to GUI even if SC_QT is off?

@mossheim
Copy link
Contributor

These are a lot of changes!!!. Do you want me to do all of them or perhaps I should close this PR and let you implement them?

please, i would prefer that you implement these changes. it's about 5 lines of code and moving 1 or 3 files, and you can easily test the result if you have a command like diff on your system. i tried to be as clear as possible in my description so that you could do it in this PR. let me know if you need any more details.

May be another solution could be just dont change anything when SC_QT is off?
Would it be harmful to have help and Classes related to GUI even if SC_QT is off?

help -- GUI related help files are already installed, and that's fine. there is not an easy way to exclude those files anyway at the moment. right now the effect of using this regex is to prevent installation of a more or less arbitrary subset of help files that have "GUI" in the name.

classes -- GUI class files absolutely cannot be installed, because any file installed that has an initClass which references GUI primitives will break class library compilation. the easiest solution, which we have already done in almost all cases, is to just separate out anything which uses GUI classes and not install it.

… when NOT SC_ABLETON_LINK by having them in GUI or Ableton folders. Allow GUI help files to install.
@sonoro1234
Copy link
Contributor Author

Done!! (I think so)

Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

thanks! just one formatting change

CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

thanks!

@mossheim mossheim merged commit 85aeec1 into supercollider:develop Jul 1, 2020
@sonoro1234 sonoro1234 deleted the guidesnotgui branch July 2, 2020 08:11
@patrickdupuis patrickdupuis mentioned this pull request Jul 5, 2020
4 tasks
mossheim added a commit that referenced this pull request Jul 7, 2020
@patrickdupuis patrickdupuis moved this from in progress to DONE in Patch release cherry-picks Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: build CMake build system comp: help schelp documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants