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

xcode-build-server: new port #22707

Merged
merged 1 commit into from
Feb 24, 2024

Conversation

woolsweater
Copy link
Contributor

Description

xcode-build-server provides a Build Server Protocol implementation that allows sourcekit-lsp to be used with Xcode projects. It recently published a v1.0.0 release.

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

macOS 13.6.1 22G313 arm64
Xcode 15.0.1 15A507

macOS 13.6.3 22G436 arm64
Xcode 15.2 15C500b

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?
  • checked your Portfile with port lint --nitpick?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

Copy link
Member

@pmetzger pmetzger left a comment

Choose a reason for hiding this comment

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

Thanks so much for the submission! A couple of comments.

PortGroup python 1.0

github.setup SolaWing xcode-build-server 1.0.0 v
name xcode-build-server
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is the default given that it's the same as the github.setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

sha256 08ea2b9a892670bb7c2f8ff90a97ae0be519744f42ba713bb1a56e247c77e885 \
size 18673

python.default_version 39
Copy link
Member

Choose a reason for hiding this comment

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

Our default version of python now is 3.12, can this package use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that should be fine. I included this because it's the lowest version that xcode-build-server says it supports; I wasn't sure how MacPorts preferred to handle that. I will change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@pmetzger
Copy link
Member

@reneeotten Not sure if you want to have a quick look too.

Copy link
Contributor

@reneeotten reneeotten left a comment

Choose a reason for hiding this comment

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

a few comments but I am not behind a computer, so can't check much.

version 1.0.0
revision 0
platforms {darwin}
license MIT
Copy link
Contributor

Choose a reason for hiding this comment

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

"darwin" is the default but if this is the "usual" Python it could be "{darwin any} if it installs the same files on each OS version. Otherwise, then line can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

if it only install Python files, then it should become "platforms {darwin any}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The project is only useful on a platform where Xcode is available, so any seems inappropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

no, it isn't {darwin any} just means that the installed files will be the same on all macOS versions, which is the case here. See the description in the guide - here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I misunderstood the syntax. So this should probably be {macosx any} then, since I don't think Xcode is usable on plain Darwin.

supported_archs noarch

homepage https://github.com/SolaWing/xcode-build-server

Copy link
Contributor

Choose a reason for hiding this comment

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

this is the default set by the "github" PortGroup, please remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

build {}

destroot {
set libdir ${destroot}${prefix}/lib/${name}
Copy link
Contributor

Choose a reason for hiding this comment

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

we usually don't want to override any phases - is there nothing to be build for this package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's just a group of 7 Python files which import each other, and one of which (named xcode-build-server) is the frontend to the tool. It does not use setuptools or any kind of build script.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's great packaging by upstream....

Anyway, I see that the shebang line read: #!/usr/bin/env python3, that is certainly not correct as here you are installing it for MacPorts' version of Python 3.12. So that line in at least the xcode-build-server file will need to be changed in a post-patch phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

build {}

destroot {
set libdir ${destroot}${prefix}/lib/${name}
Copy link
Contributor

Choose a reason for hiding this comment

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

that's great packaging by upstream....

Anyway, I see that the shebang line read: #!/usr/bin/env python3, that is certainly not correct as here you are installing it for MacPorts' version of Python 3.12. So that line in at least the xcode-build-server file will need to be changed in a post-patch phase.

set libdir ${destroot}${prefix}/lib/${name}
set binpath ${destroot}${prefix}/bin/${name}
xinstall -d -m 0755 ${libdir}
xinstall -m 0755 {*}[glob ${worksrcpath}/*.py] ${libdir}
Copy link
Contributor

Choose a reason for hiding this comment

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

the Python "library" files should ideally go into something like python.pkgd (see here) with the package-name included instead of just dumping them in ${prefix}/lib/name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

xinstall -d -m 0755 ${libdir}
xinstall -m 0755 {*}[glob ${worksrcpath}/*.py] ${libdir}
xinstall -m 0755 -W ${worksrcpath} ${name} ${libdir}
file link -symbolic ${binpath} ../lib/${name}/${name}
Copy link
Contributor

Choose a reason for hiding this comment

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

the xcode-build-server script should probably not be installed in ${libdir} and then linked, but just directly in ${binpath}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it imports the other files by their bare names (e.g. import config) I believe it needs to be physically located next to them.

@pmetzger
Copy link
Member

CI is now failing. We should fix that and then I think we can merge.

@woolsweater
Copy link
Contributor Author

CI is now failing. We should fix that and then I think we can merge.

Thanks @pmetzger; it does not appear to me that the failure had anything to do with the new Portfile: https://github.com/macports/macports-ports/actions/runs/7951916696/job/21705909092?pr=22707 I just pushed an empty change to try to re-trigger the checks but they seem to have been skipped entirely. Is it possible for you to trigger them?

@woolsweater
Copy link
Contributor Author

Is it possible for you to trigger them?

Never mind; I added a bit to the commit message and pushed that.

build {}

post-patch {
reinplace "s|#!/usr/bin/env python3|#!/usr/bin/env python${python.branch}|" \
Copy link
Contributor

Choose a reason for hiding this comment

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

please do the reinplace to change it to "${python.bin}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

${python.bin} results in #!/usr/bin/env python/opt/local/Library/Frameworks/Python.framework/Versions/3.12/bin/python3.12, which doesn't execute.

Copy link
Contributor

Choose a reason for hiding this comment

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

what I mean is that you should replace "/usr/bin/env python3" with "{python.bin}", that will work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, got it.

Copy link
Contributor

Choose a reason for hiding this comment

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

so, this should become:

post-patch {
    reinplace "s|/usr/bin/env python3|${python.bin}|" ${worksrcpath}/${name}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


destroot {
set pkgdir [string replace ${python.pkgd} 0 [string length ${prefix}]]/${name}
set destpkgdir ${destroot}${prefix}/${pkgdir}
Copy link
Contributor

Choose a reason for hiding this comment

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

creating two new variables here seem unnecessary. I can take a look later tonight when I am behind a computer; this should be doable with variables that are already defined in the "python" PortGroup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unable to find a suitable predefined variable but I would be happy to take your suggestions. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

the destroot section should become:

destroot {
    xinstall -d ${destroot}/${python.pkgd}/${name}
    xinstall -m 0755 {*}[glob ${worksrcpath}/*.py] ${destroot}/${python.pkgd}/${name}
    xinstall -m 0755 -W ${worksrcpath} ${name} ${destroot}/${python.pkgd}/${name}
    ln -s ${python.pkgd}/${name}/${name} ${destroot}${prefix}/bin/${name}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. So that I understand for next time, would you mind letting me know why repeating the literal ${destroot}/${python.pkgd}/${name} is preferable to making a variable for it and using that?

Copy link
Contributor

@reneeotten reneeotten left a comment

Choose a reason for hiding this comment

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

@woolsweater please take a look at the suggested changes, with these it appears to me that everything is working as intended. Please verify and if that's the case make the changes and push to this PR.

@woolsweater
Copy link
Contributor Author

@woolsweater please take a look at the suggested changes, with these it appears to me that everything is working as intended. Please verify and if that's the case make the changes and push to this PR.

Done.

`xcode-build-server` provides a Build Server Protocol implementation
that allows `sourcekit-lsp` to be used with Xcode projects.

The project does not provide an install script or configuration, so the
files must be manually moved into appropriate locations.
@reneeotten reneeotten merged commit d92e3ab into macports:master Feb 24, 2024
4 checks passed
@woolsweater
Copy link
Contributor Author

Thanks, @reneeotten !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants