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

QLColorCode: update to 4.1.0 #9138

Merged
merged 1 commit into from
Jan 24, 2021
Merged

Conversation

Tatsh
Copy link
Contributor

@Tatsh Tatsh commented Nov 15, 2020

Description

Update QLColorCode to 3.1.1 with a few patches. One of the patches fixes selecting the theme.

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 11.0.1 20B29
Xcode 12.2 12B45b

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL?
  • checked your Portfile with port lint?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?

@Tatsh
Copy link
Contributor Author

Tatsh commented Nov 15, 2020

It does not work yet:

Process:               highlight [93251]
Path:                  /opt/local/bin/highlight
Identifier:            highlight
Version:               0
Code Type:             X86-64 (Native)
Parent Process:        zsh [93247]
Responsible:           quicklookd [93244]
User ID:               501

Date/Time:             2020-11-15 18:18:17.830 -0500
OS Version:            macOS 11.0.1 (20B29)
Report Version:        12
Bridge OS Version:     3.0 (14Y908)
Anonymous UUID:        D2B4728B-5895-4B5F-4156-14D75538F8BD


Time Awake Since Boot: 110000 seconds

System Integrity Protection: enabled

Crashed Thread:        0

Exception Type:        EXC_CRASH (SIGABRT)
Exception Codes:       0x0000000000000000, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Termination Reason:    DYLD, [0x6] Filesystem Sandbox

Application Specific Information:
dyld: launch, loading dependent libraries

Dyld Error Message:
  Library not loaded: /opt/local/lib/liblua.dylib
  Referenced from: /opt/local/bin/highlight
  Reason: no suitable image found.  Did find:
	file system sandbox blocked open() of '/opt/local/lib/liblua.dylib'
	/opt/local/lib/liblua.dylib: stat() failed with errno=1
	file system sandbox blocked open() of '/opt/local/lib/liblua.dylib'

Binary Images:
       0x108c55000 -        0x108d20fff +highlight (0) <6116FD84-0836-3155-943C-8C11BF347861> /opt/local/bin/highlight
       0x109820000 -        0x1098bbfff  dyld (832.7.1) <2705F0D8-C104-3DE9-BEB5-B1EF6E28656D> /usr/lib/dyld


@Tatsh
Copy link
Contributor Author

Tatsh commented Nov 15, 2020

Not sure of the cause. I've tried the following:

  • add /opt/local/bin/highlight to 'Developer Tools' in Security & Privacy
  • add /bin/zsh to 'Developer Tools' in Security & Privacy
  • add /bin/zsh to 'Full Disk Access' in Security & Privacy
  • ran qlmanage -r cache
  • ran killall Finder quicklookd

@Tatsh
Copy link
Contributor Author

Tatsh commented Nov 15, 2020

Also for some reason, my /opt/local/lib/liblua.dylib was installed with permissions 0644 instead of 0755. I fixed it now but this has to be coming from the lua package.

@Tatsh
Copy link
Contributor Author

Tatsh commented Nov 15, 2020

Restarting had no effect on the issue.

@Tatsh
Copy link
Contributor Author

Tatsh commented Nov 16, 2020

Big Sur issue: anthonygelibert/QLColorCode#72

@ryandesign
Copy link
Contributor

Any reason not to merge this, other than the Big Sur issue?

@Tatsh
Copy link
Contributor Author

Tatsh commented Nov 20, 2020

I'd say it's good to go for all pre-11 systems.

I am working on a patch for upstream to fix it on Big Sur.

@Tatsh Tatsh marked this pull request as ready for review November 20, 2020 18:32
@Tatsh
Copy link
Contributor Author

Tatsh commented Nov 20, 2020

@ryandesign I could block it from macOS 11 for now since it's pointless to install on that version.

@Tatsh
Copy link
Contributor Author

Tatsh commented Dec 6, 2020

I have fixed the Big Sur issue and sent a patch to upstream: anthonygelibert/QLColorCode#73

@Tatsh Tatsh force-pushed the update-qlcolorcode branch from 4799a2e to 3c5ee44 Compare December 7, 2020 07:18
@Tatsh
Copy link
Contributor Author

Tatsh commented Dec 7, 2020

This now incorporates my patch which is in PR state in upstream. This QuickLook plugin works great now on Big Sur.

The building of Lua and Highlight is not ideal, but this project is complicated by having the Highlight dependency as a binary rather than a library. A real solution is to have the native code call the library rather than going through a shell script and linking statically with Highlight.

@macportsbot
Copy link

Travis Build #15633 Passed.

Lint results
--->  Verifying Portfile for QLColorCode
--->  0 errors and 0 warnings found.

Port QLColorCode success on xcode10.3. Log
Port QLColorCode success on xcode9.4. Log
Port QLColorCode success on xcode8.3. Log
Port QLColorCode success on xcode7.3. Log

@Tatsh Tatsh force-pushed the update-qlcolorcode branch from 3c5ee44 to 872eefd Compare December 17, 2020 08:47
@Tatsh Tatsh changed the title QLColorCode: update to 3.1.1 QLColorCode: update to 4.0.2 Dec 17, 2020
@Tatsh
Copy link
Contributor Author

Tatsh commented Dec 17, 2020

This is now a working version for Big Sur. Upstream accepted my patch.

Is it an issue if Lua must be at least deactivated for this to install correctly?

If I attempt to use the MacPorts, liblua.a it is unable to find a symbol: lua_newuserdata. I am not sure exactly how to fix Lua to have that symbol in the static lib, but the version pulled down with this port works.

@Tatsh
Copy link
Contributor Author

Tatsh commented Dec 17, 2020

I may be missing a flag on the linker: anthonygelibert/QLColorCode#78

@Tatsh Tatsh force-pushed the update-qlcolorcode branch from 872eefd to 16df4fe Compare December 17, 2020 19:05
@Tatsh Tatsh changed the title QLColorCode: update to 4.0.2 QLColorCode: update to 4.1.0 Dec 17, 2020
@Tatsh
Copy link
Contributor Author

Tatsh commented Dec 17, 2020

Updated to 4.1.0.

@g5pw
Copy link
Contributor

g5pw commented Jan 24, 2021

@Tatsh wow, great work, thank you! So, are the issues with Lua resolved in 4.1.0? Can we merge this?

@Tatsh
Copy link
Contributor Author

Tatsh commented Jan 24, 2021

This can be merged, but it will conflict with the Lua port temporarily. Solution is to deactivate Lua first and then reactivate it. port -f deactivate lua && port install QLColorCode && port -f activate lua. This is because the MacPorts static lib is missing some symbols.

@g5pw
Copy link
Contributor

g5pw commented Jan 24, 2021

So, if I understand this correctly, this port build and installs its own version of Lua and Highlight and installs them over the Lua install, requiring a port -f activate lua to restore the original libs? If so, I don't think this should be merged yet.

If a vendorized copy of highlight and lua is needed, they should be installed in a separate dir, and if they're only a build dependency and statically linked, the libs should not be installed at all.

Additionally, rebuilding (and adding a hefty boost dependency) highlight just to have it in the bundle is probably unnecessary? From what I understand from the patch you sent upstream, QLColorCode is not linked to highlight, it calls the binary, the only change is that it's bundled together. Is my understanding correct?

@Tatsh
Copy link
Contributor Author

Tatsh commented Jan 24, 2021

Lua only has to be deactivated because the linker needs to not see it during build.

@Tatsh
Copy link
Contributor Author

Tatsh commented Jan 24, 2021

So, if I understand this correctly, this port build and installs its own version of Lua and Highlight and installs them over the Lua install, requiring a port -f activate lua to restore the original libs? If so, I don't think this should be merged yet.

If a vendorized copy of highlight and lua is needed, they should be installed in a separate dir, and if they're only a build dependency and statically linked, the libs should not be installed at all.

Additionally, rebuilding (and adding a hefty boost dependency) highlight just to have it in the bundle is probably unnecessary? From what I understand from the patch you sent upstream, QLColorCode is not linked to highlight, it calls the binary, the only change is that it's bundled together. Is my understanding correct?

Because the way bundles and code signing work on Big Sur, the copy of Highlight must be bundled in the QLColorCode bundle. Even if /opt/local/bin/highlight is signed, it still cannot use it.

@Tatsh
Copy link
Contributor Author

Tatsh commented Jan 24, 2021

Perhaps it's one more thing then: the Highlight source this pulls in needs to be patched to not see liblua.la from MacPorts. Then the conflict will be resolved.

@g5pw
Copy link
Contributor

g5pw commented Jan 24, 2021

Thank you for taking the time to explain this properly. Maybe the way to go here should be:

  1. If QLColorCode is not requiring a special version of highlight, maybe it would be sufficient to copy it into the bundle?
  2. Why is the symbol missing in the macports lua libraries? perhaps it's not the correct lua version? Also, it looks like lua_newuserdata is a macro and not a symbol.

@Tatsh
Copy link
Contributor Author

Tatsh commented Jan 24, 2021

If QLColorCode is not requiring a special version of highlight, maybe it would be sufficient to copy it into the bundle?

It doesn't need a specific version, but it does need it to be within the bundle and signed. I've taken the other route that's more compatible with the upstream project by getting the Highlight source and building it within the Xcode build process. This guarantees the highlight binary gets signed properly.

How to sign the already built thing for this? I know we can use codesign commands but I'm not an expert on that. It's more tricky than what's in this Portfile already.

Why is the symbol missing in the macports lua libraries? perhaps it's not the correct lua version? Also, it looks like lua_newuserdata is a macro and not a symbol.

This is the error if lua port is installed and activated while building:

c++ -ldl -o highlight arg_parser.o cmdlineoptions.o main.o help.o -L. -lhighlight ../../lua/liblua.a
Undefined symbols for architecture x86_64:
  "_lua_newuserdata", referenced from:
      highlight::SyntaxReader::load(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, highlight::OutputType) in libhighlight.a(syntaxreader.o)
      Diluculum::PushLuaValue(lua_State*, Diluculum::LuaValue const&) in libhighlight.a(LuaUtils.o)
     (maybe you meant: _lua_newuserdatauv)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [cli] Error 1
make: *** [cli] Error 2

This is a header conflict, not library. I can prove this by putting these lines near the end of /opt/local/include/lua.h:

LUA_API void *(lua_newuserdatauv) (lua_State *L, size_t sz, int nuvalue);
#define lua_newuserdata(L,s)    lua_newuserdatauv(L,s,1)

Then it builds fine with lua activated. So it's an include path issue.

@Tatsh Tatsh force-pushed the update-qlcolorcode branch from 16df4fe to 091ba80 Compare January 24, 2021 12:45
@Tatsh
Copy link
Contributor Author

Tatsh commented Jan 24, 2021

I fixed the include path issue with a patch and I am submitting this to upstream. This port will work without having to deactivate lua.

@g5pw
Copy link
Contributor

g5pw commented Jan 24, 2021

How to sign the already built thing for this? I know we can use codesign commands but I'm not an expert on that. It's more tricky than what's in this Portfile already.

Hm, yes, this looks trickier than I thought.

I fixed the include path issue with a patch and I am submitting this to upstream. This port will work without having to deactivate lua.

All right then, splendid work, thank you @Tatsh. Is this PR ready to merge with the latest changes, or should we wait for upstream?

@Tatsh
Copy link
Contributor Author

Tatsh commented Jan 24, 2021

This is ready. I don't think upstream will make a new release just for my build fix.

@g5pw
Copy link
Contributor

g5pw commented Jan 24, 2021

Ok, merging then. Thanks for your contribution to MacPorts, @Tatsh!

@g5pw g5pw merged commit 43046ef into macports:master Jan 24, 2021
@Tatsh Tatsh deleted the update-qlcolorcode branch January 24, 2021 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants