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

fpc: fix os.version bug #22460

Merged
merged 1 commit into from
Feb 4, 2024
Merged

Conversation

kamischi
Copy link
Contributor

os.version is the darwin version. macos_version is the macOS version.

Description

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

macOS 13.6.4 22G513 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?
  • referenced existing tickets on Trac with full URL?
  • 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?

@kamischi
Copy link
Contributor Author

kamischi commented Jan 30, 2024

I could not test, but to the best of my understanding of the docs, I expect this fixing builds on 10.9, 10.10 and 10.11.

@@ -263,7 +263,7 @@ if {${subport} eq "${name}"} {

if {${os.major} >= 11} { # Big Sur
Copy link
Contributor

Choose a reason for hiding this comment

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

This instance of os.major isn't right either, and neither is the one later in the file. Big Sur isn't Darwin 11; it's Darwin 20. The check for Ventura at the top of the file is wrong too; Ventura isn't Darwin 13; it's Darwin 22.

As I explained in #19837, it is customary to use os.major, not os.version or macos_version.

Also, verify that ${os.platform} eq "darwin" before comparing os.major.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ryandesign In its existing form the port should be set to macosx perhaps, and no need to check for darwin, since it does not support BSD and Linux, which need separate bootstrap compilers.

Copy link
Contributor Author

@kamischi kamischi Jan 31, 2024

Choose a reason for hiding this comment

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

As pointed out by barracuda156, the bootstrap compiler makes the package actually macOS only. Correct me, if I am wrong, but I should change platform from darwin to macosx.

Regarding version numbers:
I find the use of the Darwin version very clumsy. It is hard enough to keep track of the relation between name and version number of the macOS version and the Darwin version adds another layer of complexity on top of that. You say, it is customary to use. I see no benefit to use this custom here, rather the contrary. According to the MacPorts Guide using macos_version_major does exactly, what I want.
Correct me if I understand this wrong, but the direct comparison should work with versions > 11 and the use of vercmp is required for 10.x only.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kamischi
10.5 = 9
10.6 = 10
10.7 = 11
etc.
There is no need for vercomp, just use os.major.

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 am lazy and bad in memorizing. So, I would have to add that list as a note to the package description, or google it whenever I need it, or yada, yada ... Maybe this makes you understand, what I mean by clumsy 😉
No offence intended.

@ryandesign
Copy link
Contributor

You haven't increased any of the revisions… do you need to?

If the problems you are fixing here prevented the ports from building, then you don't need to increase the revisions.

If the ports built before but what got built and installed was wrong in some way, then you need to increase the revisions.

@@ -263,7 +263,7 @@ if {${subport} eq "${name}"} {

if {${os.major} >= 11} { # Big Sur
set otherOptions "-WM11.0 -XR${configure.sdkroot}"
} elseif {[vercmp ${os.version} 10.9] >= 0} { # Mavericks
} elseif {[vercmp ${macos_version} 10.9] >= 0} { # Mavericks
Copy link
Contributor

Choose a reason for hiding this comment

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

As I said in another comment, I recommend checking os.major instead, and then you won't need to use vercmp.

However, if vercmp needs to be used, [vercmp ${macos_version} 10.9] >= 0 the old complicated syntax. The newer less-complicated syntax is [vercmp ${macos_version} >= 10.9]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh. Very nice. Thanks.

@barracuda156
Copy link
Contributor

barracuda156 commented Jan 31, 2024

@kamischi As Ryan suggests, it will be much easier if you use os.major everywhere, which is a Darwin version corresponding to a given macOS. There does not seem to be a need in using alternative checks.
Then it will also be easier to merge, and I hope it is merged quickly, so I can do remaining fixes.
(I saw this problem yesterday but intended to fix it together with ppc stuff.)

@barracuda156
Copy link
Contributor

@kamischi On a side note (unrelated to this PR): do you know any easy way to a) turn off strip for a specific package and b) force debug build of FPC? I spent a whole day trying to fix it yesterday, and nothing worked. Debug target is broken, strip flags are mystically forced even when patched out from all three makefiles for the packages etc.

@kamischi
Copy link
Contributor Author

@kamischi On a side note (unrelated to this PR): do you know any easy way to a) turn off strip for a specific package and b) force debug build of FPC? I spent a whole day trying to fix it yesterday, and nothing worked. Debug target is broken, strip flags are mystically forced even when patched out from all three makefiles for the packages etc.

I think you know the strip flag -Xs. This can also be in your fpc.cfg files. Probably, your problem are the explicit strip commands in Makefiles. No idea, how to switch them off reliably. Best ask on the fpc-devel mailing list, the best way to reach the people, who really know stuff.

@kamischi
Copy link
Contributor Author

You haven't increased any of the revisions… do you need to?

If the problems you are fixing here prevented the ports from building, then you don't need to increase the revisions.

If the ports built before but what got built and installed was wrong in some way, then you need to increase the revisions.

The fixes do not change anything for versions, for which the package builds, only those for which the builds failed.
→ No need to increase the revision. At least one thing, I got right ;-)

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.

why not just using os.major but with the correct Darwin versions (see here) as we do in every other Portfile?

[edit: I see I am late to the party... but see that Ryan pointed out the same; so please just change it]

@barracuda156
Copy link
Contributor

@kamischi Just noticed, why no i386 support? Could you perhaps add it? Or is there some problem preventing it?

@kamischi
Copy link
Contributor Author

kamischi commented Feb 2, 2024

why not just using os.major but with the correct Darwin versions (see here) as we do in every other Portfile?

[edit: I see I am late to the party... but see that Ryan pointed out the same; so please just change it]

Why are you not reading my post from above?
Just to state my arguments again as clearly and concise as possible:

  • using os.major implies another level of indirection, which makes its use clumsy and error prone.
  • When reviewing the package description, it is so much more helpful to have the macOS version present and not the Darwin version. Mistakes and errors are visible on first glance.
  • why should I remember this indirection while the system can take care of it.
  • why shouldn't I use, what is listed in the docs? with no obvious difference to os.major.
  • I would like to revert the argument of "as is custom" and "as we do in every other Portfile" to "The need to start to change this practise is evident"
  • Why is my preference as a maintainer of the package not respected, but rather people come back at me again and again, without reference to any of my previous arguments with a single word.

In summary, I am sick of arguing, giving in, won't argue any longer and are going to change it. Just to make it clear: You did not convince me with any arguments and it adds to the impression about MacPorts being ruled via stubborn authoritism rather than a supporting atmosphere. You simply require changes to my package description without addressing any of my arguments. I strongly question, whether this is required to keep up the quality of MacPorts. I can understand previous cases, where maintainers with less patience than me simply stopped to contribute to MacPorts.

I owe reneeotten respect and credit for being the first one pointing out the extremely helpful wiki page with the table of version numbers. MacPorts is the one and only place, where I need the Darwin number. Since this happened about once a year only, I had forgotten all about it and needed to ask Google every time, ...

Michael.

@kamischi
Copy link
Contributor Author

kamischi commented Feb 2, 2024

@kamischi Just noticed, why no i386 support? Could you perhaps add it? Or is there some problem preventing it?

The bootstrap compiler is no universal binary, x86_64 only. There is a universal i386,powerpc one, but it is a pre-3.0 version only, version 2.6-something. For supporting i386, your method for extraction of the bootstrap compiler from the disk image will also be required. That is actually, why I am also strongly interested in your contributions.

@barracuda156
Copy link
Contributor

@kamischi Just noticed, why no i386 support? Could you perhaps add it? Or is there some problem preventing it?

The bootstrap compiler is no universal binary, x86_64 only. There is a universal i386,powerpc one, but it is a pre-3.0 version only, version 2.6-something. For supporting i386, your method for extraction of the bootstrap compiler from the disk image will also be required. That is actually, why I am also strongly interested in your contributions.

Okay, I can add it, I guess.

By the way, I verified that the identical strip error happens on 10.5.8 with pas2js, both with native Apple strip and Macports cctools one, so I am pretty sure now it is a genuine upstream bug and not something gone wrong on my 10.6.

@reneeotten
Copy link
Contributor

@kamischi I am not convinced about all your personal opinions. If you want to co tribute to the project you will have to follow the conventions / best practices set by the project. You can do whatever you want in your local tree but if you want something to get merged into the project, please just make the changes.

@barracuda156
Copy link
Contributor

@kamischi While on a general note I do think that a more welcoming atmosphere would be beneficial for the development of the project, and agree that in certain cases contributions happen to be discouraged for no good reason, here specifically it is just a matter of stylistic convention. You will have exactly the same functionality of the code, nothing gets compromised by switching to os.major.
Also, consider that someone wants to add support for FreeBSD later into the port; in that case macos versions will not be defined, so extra macros will be needed – and why not just add checks for darwin now itself.

@pmetzger
Copy link
Member

pmetzger commented Feb 2, 2024

@kamischi So this is a perpetual problem in any project, right? People have conventions of various kinds, whether indentation or what code calling conventions look like or what have you, and sometimes they're good and sometimes they're bad, but generally speaking, the right thing to do is not to insist that a particular contribution deviate heavily from local norms, because the maintenance cost of non-uniformity is pretty high.

It might be true that this is a bad convention, and that might be a reasonable discussion to have on the -dev mailing list. Maybe everything should be using a different convention. But, for now, it is best if code follows the conventions people are used to, and to argue about whether the convention is good or bad separately from a given pull request.

@kamischi
Copy link
Contributor Author

kamischi commented Feb 2, 2024

Please take the following with a grain of salt.

@kamischi While on a general note I do think that a more welcoming atmosphere would be beneficial for the development of the project, and agree that in certain cases contributions happen to be discouraged for no good reason, here specifically it is just a matter of stylistic convention.

As a package maintainer, I would like to use my style. In particular as long as my style does not violate rules, whereby I expect such rules to be stated clearly in the docs.

@kamischi You will have exactly the same functionality of the code, nothing gets compromised by switching to os.major.

Let me exaggerate: When you are programming, are you typing in hex code or assembler? Oh, you are even using a high level programming language! Doesn't have hex code the same functionality? ;-)

@kamischi Also, consider that someone wants to add support for FreeBSD later into the port; in that case macos versions will not be defined, so extra macros will be needed – and why not just add checks for darwin now itself.

The first time, that someone comes up with an actual argument better than "We always do it this way." Credit for this. Thinking about it, I would argue. I see this not happening, I mean FreePascal for FreeBSD via MacPorts. As much as I know, FreePascal has direct support of FreeBSD. But if it would actually happen, I would have to work on it anyways and would then change it.

Anyways. I have already given in. In the end, having fpc as a package in MacPorts is more important to me than all of above.

Michael.

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.

@kamischi this is getting rather annoying now... I will try one more time to ask you to make changes that have been requested before (and not to add 20 lines of useless comments).

If you don't want to comply to these requests, fine with me - you can just close the PR and I at least will not engage with you anymore and attempt to get PRs merged. This will be my last response here.

# Mac OS X 10.6 Snow Leopard 10
# Mac OS X 10.5 Leopard 9
# Mac OS X 10.4 Tiger 8

Copy link
Contributor

Choose a reason for hiding this comment

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

remove all these comments; they are in the link I gave you.

# Mac OS X 10.5 Leopard 9
# Mac OS X 10.4 Tiger 8

if {${os.major} >= 22} { # 13, Ventura, fix obsolete linker options
Copy link
Contributor

Choose a reason for hiding this comment

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

check for "os.platform" as well; here and in other places as asked before.

@pmetzger
Copy link
Member

pmetzger commented Feb 3, 2024

I think we can find a way to get this shepherded through?

@kamischi
Copy link
Contributor Author

kamischi commented Feb 3, 2024

@barracuda156 By the way, I verified that the identical strip error happens on 10.5.8 with pas2js, both with native Apple strip and Macports cctools one, so I am pretty sure now it is a genuine upstream bug and not something gone wrong on my 10.6.

Since pas2js is just a utility in directory utils, you might skip building utils, by replacing make all and make install all by make compiler rtl packages and make compiler_install rtl_install packages_install. Maybe make utils_debug helps or building pas2js with old-style simple fpc commands. In general, fpc needs much less options than C compilers.

Can we transfer this discussion to a more appropriate place?

@barracuda156
Copy link
Contributor

@barracuda156 By the way, I verified that the identical strip error happens on 10.5.8 with pas2js, both with native Apple strip and Macports cctools one, so I am pretty sure now it is a genuine upstream bug and not something gone wrong on my 10.6.

Since pas2js is just a utility in directory utils, you might skip building utils, by replacing make all and make install all by make compiler rtl packages and make compiler_install rtl_install packages_install. Maybe make utils_debug helps or building pas2js with old-style simple fpc commands. In general, fpc needs much less options than C compilers.

Can we transfer this discussion to a more appropriate place?

Yes, sure. Please finalize this one so it is merged, and I will rebase a PR for PowerPC support and open a ticket for pas2js.

@kamischi
Copy link
Contributor Author

kamischi commented Feb 3, 2024

To the best of my knowledge, the last force-push should take care of all requests.

@barracuda156
Copy link
Contributor

@reneeotten @pmetzger Are we good to merge this? (I need that for my PR, so that it can be rebased against it.)

@pmetzger
Copy link
Member

pmetzger commented Feb 3, 2024

I think it's okay but @reneeotten got rather frustrated and I don't know if he's willing to confirm everything he wanted is now here.

@kamischi
Copy link
Contributor Author

kamischi commented Feb 3, 2024

It was not at all my intention to annoy or frustrate anyone, but admit that my statements can be taken that way. Sorry for that.

@barracuda156
Copy link
Contributor

@reneeotten Could you please review this?

@pmetzger
Copy link
Member

pmetzger commented Feb 3, 2024

@barracuda156 He may have been pushed past his patience. Which is unfortunate.

People should understand that getting lots of pull requests processed takes time and effort, and making it harder for those who are trying to get the work done doesn't make them happier and more likely to want to do the job. When people make requests like "please stick to the usual way of doing things", "please don't completely reformat the file and make a substantive change in the same request", "please don't argue about how you're the only one who is ever going to have to look at the Portfile, it's not true", "please argue about the way things are done on the mailing list, not with me", etc., it's not that we're trying to be pains in the ass, its that we're trying to get dozens of these things dealt with. "Why don't you just fix the problem yourself?" "Because you're making one pull request and I'm trying to shepherd lots and lots of them, please just be nice and change that thing."

I generally try to be pretty patient myself, but I get the frustration he was likely feeling. (There's one person in particular whose requests I no longer read because I'm uninterested in constantly both in being told "no, no, the CI is broken, not my Portfile", only to find over and over that the CI is not, in fact, broken, that their package just won't compile because of a stupid mistake, and having said person angrily abuse me for pointing out those mistakes.)

@barracuda156
Copy link
Contributor

@pmetzger Yeah, I get that.

Yet, we still have to proceed with this somehow, with someone having commit access to approve. The PR fixes an issue affecting several Intel systems, and I think no one would genuinely prefer having a wrong code in the portfile, regardless of how frustrating was the process of arriving to a point of fixing it (that is a sunk cost by now, so it should not affect decisions).

@kencu
Copy link
Contributor

kencu commented Feb 4, 2024

@kamischi has generally been a shining beacon of reasonableness for me….love the guy.

let’s just get this done and move on :)

@pmetzger
Copy link
Member

pmetzger commented Feb 4, 2024

I can commit once I'm reasonably assured that @reneeotten is happy.

@reneeotten reneeotten merged commit 941e8e6 into macports:master Feb 4, 2024
3 checks passed
@reneeotten
Copy link
Contributor

Most people here have many other actual obligations in life besides trying to help out here, so just because I don't respond or merge something within half a day doesn't mean much. I just don't feel like spending my limited time on a PR from someone that, at least in this specific case, has seemingly no interest in making requested changes in order to get things merged.

But yes, as @kencu said I (and we all) appreciate @kamischi contributions and I am sure that if we would start this PR over, everyone could have reacted slightly differently to get to the same outcome earlier and without as much friction. All good!

@barracuda156
Copy link
Contributor

@reneeotten Thank you!

@kamischi kamischi deleted the kms-fpc-fix-os.version branch February 4, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7 participants