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

sunshine: new Portfile #15143

Closed
wants to merge 1 commit into from
Closed

sunshine: new Portfile #15143

wants to merge 1 commit into from

Conversation

ReenigneArcher
Copy link

Description

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

macOS 12.4 21F79 x86_64
Xcode 13.4 13F17a
Command Line Tools 13.4

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 --nitpick?
  • tried existing tests with sudo port test? (tried, but errors, no tests turned on)
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?

@ReenigneArcher
Copy link
Author

I don't believe this will build on anything less than BigSur, based on this comment from the contributor who added MacOS support initially. https://github.com/SunshineStream/Sunshine/discussions/117#discussioncomment-2513494

Is that a requirement to be included in this repo?

@mascguy
Copy link
Member

mascguy commented Jun 16, 2022

I don't believe this will build on anything less than BigSur, based on this comment from the contributor who added MacOS support initially. SunshineStream/Sunshine#117 (comment)

Is that a requirement to be included in this repo?

Building on earlier versions isn't an absolute requirement, but it's preferable to do so when possible.

Based on the discussion, it sounds like hardware encoding won't be available with macOS versions prior to Big Sur. So if we are able to compile for earlier versions, we should also add a note mentioning that.

Something along the lines of:

platform darwin {
    if { ${os.major} < 20 } {
        # See: https://github.com/SunshineStream/Sunshine/discussions/117#discussioncomment-2513494
        notes-append "Port is limited to software encoding, when used with macOS releases prior to Big Sur."
    }
}

@mascguy mascguy self-assigned this Jun 16, 2022
@ReenigneArcher
Copy link
Author

Building on earlier versions isn't an absolute requirement, but it's preferable to do so when possible.

Based on the discussion, it sounds like hardware encoding won't be available with macOS versions prior to Big Sur. So if we are able to compile for earlier versions, we should also add a note mentioning that.

Okay, thanks for the quick response! I'll see what I can do to get it to compile the earlier version and report back.

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.

thanks @ReenigneArcher for the new submission. A few comments after a quick read through

multimedia/Sunshine/Portfile Show resolved Hide resolved
multimedia/Sunshine/Portfile Show resolved Hide resolved
multimedia/Sunshine/Portfile Show resolved Hide resolved
multimedia/Sunshine/Portfile Show resolved Hide resolved
multimedia/Sunshine/Portfile Show resolved Hide resolved
# bump revision when changes are made to this file
revision 1

github.setup sunshinestream sunshine master
Copy link
Contributor

Choose a reason for hiding this comment

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

this should become something like (not tested so please verify):

github.setup             sunshinestream sunshine 0.14.0 v
github.tarball_from      releases

after this change you'll need to update the checksums as well, so do sudo port clean --all ; sudo port -dv checksum in the port directory to recalculate them.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, I see upstream doesn't actually provide "releases"; so you should use "archive" instead in the above snippet.

Copy link
Author

Choose a reason for hiding this comment

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

We provide releases... but the port file will be generated before a release is published/tagged. Is it possible to use a commit instead? Also, I'm working on integration building the port inside PR checks, to ensure no breaking changes so using the commit would be the preferred option if it's possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

that sounds like the "wrong" order from our point of view ;) Automation is great, but updating a Portfile to actually use the release is what we would want - so no, we don't want to have a commit hash (unless it's a -devel port).


github.setup sunshinestream sunshine master
name Sunshine
version 0.14.0
Copy link
Contributor

Choose a reason for hiding this comment

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

this field is set automatically from the github.setup line

multimedia/Sunshine/Portfile Show resolved Hide resolved
multimedia/Sunshine/Portfile Show resolved Hide resolved
xinstall -d -m 755 ${destroot}${prefix}/etc/${name}/config

# install sunshine.conf
xinstall {*}[glob ${worksrcpath}/src_assets/common/config/*.*] ${destroot}${prefix}/etc/${name}/config
Copy link
Contributor

Choose a reason for hiding this comment

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

you probably don't want to overwrite a configuration file if someone would update to a newer version. I'm sure there are many Portfiles that take care of things like that and/or there might be a description in the guide/Wiki on how to handle this. Perhaps the phpmyadmin is one example where you can see that it checks if a configuration file is already present and depending on the outcome does a different action.

Copy link
Author

Choose a reason for hiding this comment

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

I tried to find examples but couldn't find one.

The documentation is not complete. https://guide.macports.org/#development.practices.dont-overwrite

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggested a port that does similar things to check how one could do this (i.e., phpadmin); another one that uses glob is the cppcheck port.

I agree that some parts of the guide are incomplete; sometimes you can find that information in the Wiki. For example, in this case you could check here.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, sorry I must have read it too fast previously... The link you provided is good though!

I'll use these as references:
https://trac.macports.org/browser/trunk/dports/security/stegdetect/Portfile#L49
https://trac.macports.org/browser/trunk/dports/sysutils/bacula/Portfile#L210

Copy link
Author

Choose a reason for hiding this comment

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

I'm sure I did something wrong, but tested this and it doesn't appear the post-activate works with a pkg generated using sudo port pkg sunshine

# Rename files in `destroot`
post-destroot {
    file rename ${destroot}${prefix}/etc/${name}/config/sunshine.conf ${destroot}${prefix}/etc/${name}/config/sunshine.conf.sample
    file rename ${destroot}${prefix}/etc/${name}/config/apps.json ${destroot}${prefix}/etc/${name}/config/apps.json.sample
}

# Don't overwrite existing preference files
post-activate {
    if {![file exists ${prefix}/etc/${name}/config/apps.json]} {
        file copy ${destroot}${prefix}/etc/${name}/config/apps.json.sample \
            ${prefix}/etc/${name}/config/apps.json
        }
    if {![file exists ${prefix}/etc/${name}/config/sunshine.conf]} {
        file copy ${destroot}${prefix}/etc/${name}/config/sunshine.conf.sample \
            ${prefix}/etc/${name}/config/sunshine.conf
        }
}
reenignearcher@ReenigneArchers-iMac-Pro MacOS % ls /opt/local/etc/Sunshine/config
apps.json.mp_1655322422		sunshine.conf.mp_1655322422
apps.json.sample		sunshine.conf.sample

It does work when installing using the Portfile. Is there a way to do this that will work for the pkg as well?

@ReenigneArcher ReenigneArcher marked this pull request as draft June 19, 2022 16:11
@ReenigneArcher
Copy link
Author

@mascguy

Building on earlier versions isn't an absolute requirement, but it's preferable to do so when possible.

Haven't had much luck with this, and as Catalina will be end of life in 5 months (November 30, 2022), I probably won't put in the effort to attempt to get this to compile on 10.15 where performance would be terrible anyway. I'm guessing at that point github will also remove the 10.15 runner.

@mascguy
Copy link
Member

mascguy commented Jul 8, 2022

Haven't had much luck with this, and as Catalina will be end of life in 5 months (November 30, 2022), I probably won't put in the effort to attempt to get this to compile on 10.15 where performance would be terrible anyway. I'm guessing at that point github will also remove the 10.15 runner.

That's fine, given the circumstances.

@ReenigneArcher
Copy link
Author

That's fine, given the circumstances.

Thanks. I'll try to get a new file pushed soon. I've addressed most of these concerns in the source repo (not the macports fork).

This pull request was closed.
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.

5 participants