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

make the update script work on macOS #349

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fallwith
Copy link

@fallwith fallwith commented Sep 18, 2022

macOS users run into the following issues when trying to install Skyscraper:

- They may have skipped over the need to install the qt5 and wget Homebrew packages

  • BSD tar does not support --overwrite. It also doesn't need it, as it overwrites by default
    - BSD rm doesn't support --force, but -f should work across all platforms
  • make clean won't work if this is the first time you're fetching Skyscraper, as no Makefile exists yet to define the 'clean' target
    -make will treat the VERSION file as source and fail on the invalid syntax of its contents
    - A sudden prompt for a password might be confusing or concerning

The update_skyscraper.sh script has been updated to address these things while retaining compatibility with non-macOS systems.

The following changes have been made:

- Each step of the update process has been moved into its own function

  • qmake and wget are looked for in PATH before proceeding
  • The tar command will conditionally add --overwrite whenever a non-macOS system is involved
  • rm --force has been replaced with rm -f
  • make clean is not performed unless Makefile exists
  • while building source with make under macOS, VERSION gets temporarily renamed to VERSION.bak
    • A note is echoed to STDOUT about it being the installation's use of sudo that may require a password.

The following things should NOT need to be done by macOS users:

  • Installing GNU tar on macOS
  • Changing the Qmake defaults away from clang to gcc

resolves #301

macOS users run into the following issues when trying to install
Skyscraper:

• They may have skipped over the need to install the `qt5` and `wget`
Homebrew packages
• BSD `tar` does not support `--overwrite`. It also doesn't need it, as
it overwrites by default
• BSD `rm` doesn't support `--force`, but `-f` should work across
all platforms
• `make clean` won't work if this is the first time you're fetching
Skyscraper, as no `Makefile` exists yet to define the 'clean' target
• `qmake` will treat the `VERSION` file as source and fail on the
invalid syntax of its contents
• A sudden prompt for a password might be confusing or concerning

The `update_skyscraper.sh` script has been updated to address these
things while retaining compatibility with non-macOS systems.

The following changes have been made:

• Each step of the update process has been moved into its own function
• `qmake` and `wget` are looked for in PATH before proceeding
• The `tar` command will conditionally add `--overwrite` whenever a
non-macOS system is involved
• `rm --force` has been replaced with `rm -f`
• `make clean` is not performed unless `Makefile` exists
• `VERSION` has been renamed to `VERSION.txt`
• A note is echoed to STDOUT about it being the installation's use of
`sudo` that may require a password.

The following things should NOT need to be done by macOS users:

• Installing GNU tar on macOS
• Changing the Qmake defaults away from clang to gcc
@muldjord
Copy link
Owner

Hi,
Thank you for the PR. I have not tested it, but I will if you make the following change. Change VERSION.txt back to VERSION. It is used elsewhere and not just by the installation script. And generally I find it ugly to just add .txt to files. Thank you.

- use 'VERSION' as the version file's name
- on macOS systems, rename 'VERSION' to 'VERSION.bak' temporarily to
  prevent build issues with `make`, which evidently evidently treats
  'VERSION' files as source
@fallwith
Copy link
Author

Hi,
Thank you for the PR. I have not tested it, but I will if you make the following change. Change VERSION.txt back to VERSION. It is used elsewhere and not just by the installation script. And generally I find it ugly to just add .txt to files. Thank you.

Hi @muldjord,

Sure thing, I have gone back to VERSION with b558fd3.

The problem is that make (maybe only when Clang is used?) treats the VERSION file as source and the compile process fails. Now if macOS is detected, the VERSION file gets temporarily renamed to VERSION.bak and then put back as VERSION after either a successful or failed build. I have updated the PR description.

Also... now that I am able to get Skyscraper working on my macOS system, I am really enjoying it. Thank you very much for such a great product!

@muldjord
Copy link
Owner

Hmm, I think the reason VERSION is seen as a source file needs to be investigated further. Renaming it back and forth isn't exactly optimal either (although I appreciate you at least coming up with some kind of solution) and could cause unexpected problems in niche cases.

Perhaps digging into the makefile qmake creates can help figure out what's going on.

@fallwith
Copy link
Author

@muldjord It looks like the Qmake project file directly does an include of the VERSION file, so my guess is that doing so causes VERSION to be a part of Makefile.

I dug into this one a little further. We wouldn't have to rename or temporarily move the VERSION file if we could rely on it's line putting quotes around the version value inside the file.

If I make this change to the contents of VERSION:

- VERSION=1.2.3
+ VERSION="1.2.3"

then everything works.

@muldjord
Copy link
Owner

That makes perfect sense. I will make that change. Thanks for looking into it. If you can then adjust the PR to not worry about the VERSION file, then I could test that also. Thanks.

@fallwith
Copy link
Author

I apologize, @muldjord - it seems that my earlier conclusion about simply using quotes was incorrect. They don't hurt anything, but they don't make things work. When testing from scratch, I am still running into errors getting make (Clang) to build when there is a VERSION file at PWD. Temporarily renaming it is still required. I'm not sure why I had success previously with the quotes.

I then tried changing VERSION to just hold a version number (no VERSION="") with these 3 changes:

# VERSION file
3.7.7

# updater script
VERSION=$(<$VERSION_FILE)

# Qmake project file
DEFINES+=VERSION=\\\"$(cat VERSION)\\\"

With those changes, the VERSION file is no longer listed as a source in the Makefile file that gets generated. But unfortunately, make still errors out when the VERSION file is present. So even though the include is completely gone from the QMake project file and VERSION is no longer getting listed as a source file in Makefile, make still loads the contents of VERSION and doesn't like it.

For now it seems that we must do one of the following:

  • rename VERSION to another filename to prevent conflicts
  • temporarily move VERSION to another name before running make on macOS, and then move it back (the PR currently does this)
  • create a directory for building that is not the same as the directory that holds the VERSION file. The QMake project file and Makefile would end up living in something like a "build" subdirectory, with "src" being a further subdirectory of that.

I'll let you know if I come up with any other ideas.

@fallwith
Copy link
Author

fallwith commented Oct 1, 2022

Here is some more information about why the existence of the VERSION file at PWD causes problems...

src/main.cpp:26 has #include <QtGlobal>

That include lights up the following chain of includes:

  • /usr/local/Cellar/qt@5/5.15.6/lib/QtCore.framework/Headers/QtGlobal:1
  • /usr/local/Cellar/qt@5/5.15.6/lib/QtCore.framework/Headers/qglobal.h:45
  • /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/type_traits:420
  • /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/cstddef:37

cstddef:37 has #include <version>

On macOS, the filesystem is case-insensitive by default. Therefore the #include <version> statement is bringing in the local VERSION file and causing problems.

I can currently think of the following 4 ways around this problem:

  • Temporarily move VERSION out of the way, compile, and then move it back even if there's an error. This is what the open PR does currently.
  • Rename VERSION to another name that doesn't match case-insensitively with 'version'
  • Have VERSION live somewhere other than the root of the project (anywhere but PWD at the time when make is invoked)
  • Update the contents of the VERSION file to be valid C++ code that can be slurped in with an include statement.

@josegonzalez
Copy link

Using the forked script in this PR seemed to allow things to install properly, but no artwork is being generated for any platform on an M1 Mac running Ventura.

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.

Numerous issues on Mac OS 11.2+
3 participants