dev-lang/lazarus-2.2.4: attempting to address calling gcc directly

[corvus1]

FreePascal does clunky things in Makefiles, such as:

GCCLIBDIR:=$(shell dirname `gcc -m32 -print-libgcc-file-name`)

My first naive idea is to replace gcc with ${CC}. If there are better ways of doing it, please tell me. ;-)

Bug: https://bugs.gentoo.org/818154

[gentoo-repo-qa-bot]

Pull request CI report

Report generated at: 2022-12-24 21:53 UTC
Newest commit scanned: 0e96038
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/a8b9777b6f/output.html

[gentoo-repo-qa-bot]

Pull request CI report

Report generated at: 2022-12-24 22:04 UTC
Newest commit scanned: 9bf68af
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/1782070e03/output.html

[Amynka]

Hello, thank you for the fix. We usually prefer patches. Would you mind making a patch for the makefile? Thanks :)

[corvus1]

Hello, thank you for the fix. We usually prefer patches. Would you mind making a patch for the makefile? Thanks :)

I wouldn't mind at all, there's a bit of a problem, though. I formatted a patch, https://dpaste.com/76WBW653B.txt but it's 138KB in size, which is probably a bit large for ${FILESDIR}. And it's kinda stupid, it changes the same line everywhere. I basically unpacked the sources, executed my one-liner and did git format-patch -1 on it. It feels a little inefficient.

[corvus1]

I just thought of something. While we're at it, maybe we should add QA_FLAGS_IGNORED and QA_PRESTRIPPED to close (or rather squelch) https://bugs.gentoo.org/737060
Is that okay?

[ghost]

Hello, thank you for the fix. We usually prefer patches. Would you mind making a patch for the makefile? Thanks :)

I wouldn't mind at all, there's a bit of a problem, though. I formatted a patch, https://dpaste.com/76WBW653B.txt but it's 138KB in size, which is probably a bit large for ${FILESDIR}. And it's kinda stupid, it changes the same line everywhere. I basically unpacked the sources, executed my one-liner and did git format-patch -1 on it. It feels a little inefficient.

You can place it somewhere and add it from SRC_URI. Anyway it seems better your one-line in this case

[corvus1]

You can place it somewhere and add it from SRC_URI. Anyway it seems better your one-line in this case

Yeah, that's what I originally thought, it makes very little sense to me to add a patch of that size that can be generated on the fly with just one find+sed.

[gentoo-bot]

Pull Request assignment

Submitter: @corvus1
Areas affected: ebuilds
Packages affected: dev-lang/lazarus

dev-lang/lazarus: @Amynka, voron1[at]gmail.com

Linked bugs

Bugs linked: 818154, 737060

---

In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of Conduct ● Copyright policy (expl.) ● Devmanual ● GitHub PRs ● Proxy-maint guide

[gentoo-repo-qa-bot]

Pull request CI report

Report generated at: 2022-12-26 08:54 UTC
Newest commit scanned: 0eb4431
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/dc50123c1e/output.html

[Amynka]

You can place it somewhere and add it from SRC_URI. Anyway it seems better your one-line in this case

Yeah, that's what I originally thought, it makes very little sense to me to add a patch of that size that can be generated on the fly with just one find+sed.

Hello, You can use it from here https://dev.gentoo.org/~amynka/snap/lazarus-2.2.4-makefile.patch.bz2

[corvus1]

You can place it somewhere and add it from SRC_URI. Anyway it seems better your one-line in this case

Yeah, that's what I originally thought, it makes very little sense to me to add a patch of that size that can be generated on the fly with just one find+sed.

Hello, You can use it from here https://dev.gentoo.org/~amynka/snap/lazarus-2.2.4-makefile.patch.bz2

Do you really want me to apply that instead of just the one-line command? It seems to me like a really bizarre thing to do. Not to mention, when they add a little tiny extra module to the build in the next version, the one-liner will keep working, but the patch will have to be reworked, reuploaded, re-mirrored, redistributed... Why are we doing this?

[ghost]

You can place it somewhere and add it from SRC_URI. Anyway it seems better your one-line in this case

Yeah, that's what I originally thought, it makes very little sense to me to add a patch of that size that can be generated on the fly with just one find+sed.

Hello, You can use it from here https://dev.gentoo.org/~amynka/snap/lazarus-2.2.4-makefile.patch.bz2

Do you really want me to apply that instead of just the one-line command? It seems to me like a really bizarre thing to do. Not to mention, when they add a little tiny extra module to the build in the next version, the one-liner will keep working, but the patch will have to be reworked, reuploaded, re-mirrored, redistributed... Why are we doing this?

Using find/sed would change everything without any control by you (you know what you are actually changing but you don't actually know where the changes are made). At every new version you have to verify if your sed is still valid and look if some files should not have to be changed. Making a patch will helps you to track these changes on source code. It's longer but safer compared to sedding everything

[corvus1]

Using find/sed would change everything without any control by you (you know what you are actually changing but you don't actually know where the changes are made). At every new version you have to verify if your sed is still valid and look if some files should not have to be changed. Making a patch will helps you to track these changes on source code. It's longer but safer compared to sedding everything

I sort of get that argument, and I thought about it, but in this case... really? I can come up with a more reliable regular expression, and match the -print-libgcc-file-name part, so it less likely to touch something we don't want to touch, but making a whole patch out of it? Do we need to?

[corvus1]

How about that idea?

sed -E 's#`gcc(.+print-libgcc-file-name.+)#`${CC}\1#'

But honestly, we really should match the shell dirname instead. If they call gcc directly, it doesn't really matter why they do it, to find libgcc or to do something else. We don't want that anyway, correct?

[corvus1]

Or even, just shell. If there's a shell command with gcc in it, we already don't want that.

[corvus1]

So... I don't know.
Let's make some kind of decision. It is of course at the maintainer's discretion, so @Amynka if you insist on the patch, I'll apply that. Although, I will state for the record that I didn't properly review it, and as far as I know it could also do something weird, in the exact same way as find+sed.

My original plan was to actually go to bugzilla, and simply ask Ago to re-run his automated check, to make sure it actually removes all direct calls to gcc, actually achieving the goal of all these fiddlings. As far as verification goes, seemed simple enough.

It's just dragging on. And the bug seems minor and not really affecting anyone. While I have new and exciting changes planned and prepared in my little overlay.

The next phase was going to be to add libqt5pas-9999, lazarus-9999, and, hopefully, double-commander-9999, if my other PR gets merged. And after that, I was planning to propagate changes to stable. Seeing that stable lazarus is still gtk2-only. The plan that I had was to just stabilize lazarus-2.2.4 and drop old (and the old is really old, by the way). So, Amy, if you approve of the plan, I will line up the changes and fire up more PRs, as soon as we're done with what automated systems managed to dig up. :-)

[Amynka]

So... I don't know. Let's make some kind of decision. It is of course at the maintainer's discretion, so @Amynka if you insist on the patch, I'll apply that. Although, I will state for the record that I didn't properly review it, and as far as I know it could also do something weird, in the exact same way as find+sed.

My original plan was to actually go to bugzilla, and simply ask Ago to re-run his automated check, to make sure it actually removes all direct calls to gcc, actually achieving the goal of all these fiddlings. As far as verification goes, seemed simple enough.

It's just dragging on. And the bug seems minor and not really affecting anyone. While I have new and exciting changes planned and prepared in my little overlay.

The next phase was going to be to add libqt5pas-9999, lazarus-9999, and, hopefully, double-commander-9999, if my other PR gets merged. And after that, I was planning to propagate changes to stable. Seeing that stable lazarus is still gtk2-only. The plan that I had was to just stabilize lazarus-2.2.4 and drop old (and the old is really old, by the way). So, Amy, if you approve of the plan, I will line up the changes and fire up more PRs, as soon as we're done with what automated systems managed to dig up. :-)

Thats certainly not good.. Then we have to review to patch and worst case I will have to re-upload it :)

[corvus1]

Thats certainly not good.. Then we have to review to patch and worst case I will have to re-upload it :)

I couldn't quite tell if you're kidding at first, I think you got me there.
I pushed the changes.
Should we (can we?) maybe push it to the tree, maybe unkeyworded at first (?), and ask Agostino to re-run his CI tests to make sure it achieves the goal? Or revbump it? What's the normal practice here?

[gentoo-repo-qa-bot]

Pull request CI report

Report generated at: 2022-12-30 18:09 UTC
Newest commit scanned: 9b36335
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/9738593159/output.html

[Amynka]

@corvus1 Hello, Sorry for late response.

I suggest we review properly the patch as mentioned.. We test it and revbump it and then commit it to the tree and that will close the bug...

[corvus1]

@corvus1 Hello, Sorry for late response.

I suggest we review properly the patch as mentioned.. We test it and revbump it and then commit it to the tree and that will close the bug...

I don't quite understand how we can review a 160KB patch generated by sed.
I suggest we test it instead. I tested compilation, and it works, but I am not entirely sure how to verify that it fixes the original issue of calling gcc directly. The only thing I can think of is committing it to the tree and asking Ago to re-run the tests that discovered the issue in the first place.

If you have a better idea, I'd love to learn about it.

[Amynka]

@corvus1 Well right now i could think of at least one way.. For example: One uses the patch and then checks the files again in workdir with grep if there is other gcc calls or if it was eliminated .. I can check that out as well.. Anyhow for sure we revbump..

[corvus1]

find -type f -name "Makefile" -exec grep "\`gcc" {} \;

In workdir returns empty. Did you think sed would miss anything? :-)

[Amynka]

@corvus1 I suggest you switch to more friendly tone :)

[corvus1]

@corvus1 I suggest you switch to more friendly tone :)

That wasn't unfriendly. Sorry if it came across that way. I was joking.

[gentoo-repo-qa-bot]

Pull request CI report

Report generated at: 2023-01-10 23:19 UTC
Newest commit scanned: c511702
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/e890f6cb65/output.html

[corvus1]

So, I grepped the workdir as you suggested, and the gcc calls I was aiming to remove are of course all removed. However, there are of course other mentionings of the word "gcc" in the makefiles. And there's a lot of them. It's just impossible to check them all by hand. This is what automated checks are for, right? The system that detected the problem to begin with. So I suggest we go with what we have now, and see if the system reproduces the problem again, or it is fixed.

The only thing is, I'm not sure we should actually close the bug. Maybe just reference it. Or maybe it's not important, and it will reopen if it reproduces the issue again. I'm not sure.