edid-decode: update to 20230831

[wwalexander]

Also, add configure and compiler options to specify C++11 on older systems as detailed in https://trac.macports.org/ticket/67934

Description

Type(s)

• bugfix
• enhancement
• security fix

Tested on

macOS 13.5.1 22G90 arm64
Xcode 14.3.1 14E300c

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 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?

[ryandesign]

You've also completely reformatted the portfile's whitespace, which makes it hard to see the non-whitespace changes. If you want to change the whitespace, please separate this into two commits: one that only changes the whitespace, and one that only does non-whitespace changes. If you're changing the whitespace, please add a blank line after the modeline.

You've added -std=c++11 to CXXFLAGS but as I said in the ticket this will do nothing until the makefile is fixed to actually use CXXFLAGS. You can see by reading the build log that -std=c++11 has not actually been passed to the compiler.

[wwalexander]

You've also completely reformatted the portfile's whitespace, which makes it hard to see the non-whitespace changes.

I did this because I was instructed on a previous PR that the whitespace in the Portfile must be aligned to a tabstop of 4.

If you want to change the whitespace, please separate this into two commits: one that only changes the whitespace, and one that only does non-whitespace changes. If you're changing the whitespace, please add a blank line after the modeline.

I was under the impression that commits must be squashed for a PR, is this not the case?

You've added -std=c++11 to CXXFLAGS but as I said in the ticket this will do nothing until the makefile is fixed to actually use CXXFLAGS. You can see by reading the build log that -std=c++11 has not actually been passed to the compiler.

Should I patch the Makefile for now, or should I wait until an upstream fix is applied for this update?

[wwalexander]

@ryandesign I've sent a patchfile to the maintainer Hans Verkuil to use CXXFLAGS instead of CFLAGS.

For now, to keep things clean I've created a new branch that simply revises the existing version (20230804) to specify C++11 mode, apply the CXXFLAGS patchfile, and add a blank line after the modeline to satisfy port lint --nitpick. Does this look more acceptable? If so, then I can create a new PR for it, close this one, and once the fixed version is merged I can create a separate PR for the version update.

Also, does the entire Portfile need to use the same alignment for columns, or only within each blank-line-delimited "section"? If the former, then I can create a further PR to adjust the tabstops to match each other.

[pmetzger]

I was under the impression that commits must be squashed for a PR, is this not the case?

You should squash any commits that aren't relevant to someone looking at the changes in the future, like tweaks or fixes people request. Whitespace modifications should indeed be made in a distinct commit if possible so people can look at the substantive changes more easily.

[wwalexander]

You should squash any commits that aren't relevant to someone looking at the changes in the future, like tweaks or fixes people request. Whitespace modifications should indeed be made in a distinct commit if possible so people can look at the substantive changes more easily.

Awesome, thank you for the guidance! Should I create a new clean PR as I outlined here or just amend this PR?

[pmetzger]

You can just force push a new set of commits onto this PR. It's better that way so that the history of the request is available to those reviewing it.

[reneeotten]

ping @wwalexander

[ryandesign]

You've also completely reformatted the portfile's whitespace, which makes it hard to see the non-whitespace changes.

I did this because I was instructed on a previous PR that the whitespace in the Portfile must be aligned to a tabstop of 4.

I wouldn't say "must", but it is strongly preferred. It is what the modeline says. And the port was already aligned at 4-space tabstops before your changes.

If the only reason why you were increasing the width of column 1 was to accommodate the length of "configure.cxxflags-append" then the usual solution is to wrap that line instead, e.g.:

configure.cxxflags-append \
                        -std=c++11

If you want to change the whitespace, please separate this into two commits: one that only changes the whitespace, and one that only does non-whitespace changes. If you're changing the whitespace, please add a blank line after the modeline.

I was under the impression that commits must be squashed for a PR, is this not the case?

As it says in the PR checklist, commits should be minimized. Per the linked documentation in the guide, that means "one commit per logical change". If your PR contains one logical change (e.g. update a port to a new version), there should be one commit for that. We don't want, for example, to see a PR where you submit an update, CI fails, and then you make another commit to fix the CI failure. Those two should be squashed into one since they are part of the single logical task of updating the port. However, if you have multiple logical changes in your PR, such as in this case 1. updating the port and 2. adjusting whitespace, there should be multiple separate commits. Often, multiple obvious small logical commits will be combined into one commit, but extensive whitespace changes should always be in a commit by themselves because otherwise you cannot easily see from a diff what non-whitespace changes were made.

You've added -std=c++11 to CXXFLAGS but as I said in the ticket this will do nothing until the makefile is fixed to actually use CXXFLAGS. You can see by reading the build log that -std=c++11 has not actually been passed to the compiler.

Should I patch the Makefile for now, or should I wait until an upstream fix is applied for this update?

If you can figure out a patch that works, you can add it here. Put the URL where you submit the patch to the maintainer at the top of the patchfile so that it can be followed up on later.

If the developer already has a patch for it, you can use theirs, and put the URL where you found it at the top of the patchfile.

[wwalexander]

I've created a new set of commits that's hopefully more readable and useful.

If you can figure out a patch that works, you can add it here.

I've added a patch to this port to make the Makefile use CXXFLAGS.

Put the URL where you submit the patch to the maintainer at the top of the patchfile so that it can be followed up on later.

I originally sent the patch directly to the author's email, but it may have gotten lost in the shuffle. Waiting on approval to join the project's mailing list but I'll send the patch there and add a link within the patchfile as soon as I can.