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

Fix compiler errors #4370

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

robert-werner
Copy link

Several issues like this (#4359 (comment)) were opened, and fixes were proposed, but no PRs were created to manage these errors without patches. So I decided to create this PR.

Copy link

@luk-brue luk-brue left a comment

Choose a reason for hiding this comment

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

Are the changes in SConstruct really necessary? I read in #4243 that this was the first workaround, but the substantial changes are those in the other three files. At least this is my interpretation. I built Mapnik with only those three files changed. It builds completely, but fails many tests. Is this normal? If yes, I would approve of these changes

@robert-werner
Copy link
Author

Are the changes in SConstruct really necessary?

Yes, they are necessary. Otherwise it would fail to build, rendering mapnik un-usable for any usage.
I've built mapnik for usage in OSM server, tiles are rendering fine without errors.

@luk-brue
Copy link

luk-brue commented Sep 16, 2023

I was able to compile completely without the removal of -fvisibility-type flags from default compiler flags.
I did not use mapnik any further afterwards, e.g. I did not render OSM tiles with it, because of the astounding number of errors from make test. So I uninstalled and rebuilt, this time with the -fvisibilty-type flags removed. The high number of make test failures persisted, so nothing improved. So the test failures are something to address elsewhere.

Could you try to build it without changes in SConstruct and test the functionality? Here is my reasoning, which might be wrong - please correct me.

My scepticism is due to the thought that the flag -fvisibility type flags might have been added with a reason in mind that we don't know. Note how the official documentation for visibility support mentions performance reasons. If I am mistaken, please explain to me why it would be beneficial to remove the flag from global compiler defaults.

if we just remove it, it might cause other problems that go unnoticed? The flags purpose is to set the visibility to hidden as a default value unless declared otherwise. (Explanation on SO). So usually, there is a macro to declare objects as visible, in this case it is MAPNIK_DECL.

@albertas-akistechnologies fix mentions that their fix (aka this commit) is declaring a template as visible. I don't fully understand this because I am not a programmer but even without that knowledge iI think removing the default=hidden visibility feature globally is a more drastic thing to do than to manually set the desired objects as visible.

If this is not right, please help me understand!

But if my reasoning is not wrong, my suggestion would be that we leave global compiler flags from SConstruct out of this fix and then we could merge this.

@luk-brue
Copy link

Because this has gone stale unfortunately, could you maybe review these changes @mathisloge ? It would fix some issues, and make building easier again if you are a newbie like me and don't know the workarounds. Or do you have other plans coming in for fixing the compilation errors?

@Nakaner Nakaner mentioned this pull request Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants