Skip to content

Fix older bazels on new GCCs#30655

Closed
elfprince13 wants to merge 1 commit intospack:developfrom
Geopipe:fix/bazel-ijar-limits
Closed

Fix older bazels on new GCCs#30655
elfprince13 wants to merge 1 commit intospack:developfrom
Geopipe:fix/bazel-ijar-limits

Conversation

@elfprince13
Copy link
Copy Markdown
Contributor

Looks like <limits> was being transitively included somewhere until GCC restructured its header files with GCC 11.

@adamjstewart
Copy link
Copy Markdown
Member

Has this patch been submitted upstream?

@elfprince13
Copy link
Copy Markdown
Contributor Author

There's a similar patch that's been submitted for Bazel 5+, but doesn't appear to have been backported to earlier releases.

@adamjstewart
Copy link
Copy Markdown
Member

Can we use the exact same patch or is that not sufficient? Can you add a link to that patch so we have more context?

patch('disabledepcheck_old.patch', when='@0.3.0:0.3.1+nodepfail')

# include-what-you-use violation that snuck under the radar until GCC 11
patch('ijar_limits_fix.patch', when='@0.3.2:5.0.0')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Version ranges as inclusive. If you want >= 0.3.2, < 5, that would look like 0.3.2:4

@elfprince13
Copy link
Copy Markdown
Contributor Author

Can we use the exact same patch or is that not sufficient? Can you add a link to that patch so we have more context?

It's linked in my previous comment, but was insufficient. The other change I made has also been included in a different patch-set used by the Alpine Linux ports team, but I didn't test any of the other patches in that set as they were unnecessary for successful building on my machine.

@adamjstewart
Copy link
Copy Markdown
Member

Is this a duplicate of #28548?

@elfprince13
Copy link
Copy Markdown
Contributor Author

Appears to overlap anyway. This applies a smaller set of patches more broadly. I did not need the other changes from the Alpine patch set to build the particular Bazel I cared about; however I can confirm that the patches in this PR are
(a) necessary to build with Clang 14 when using the GCC 11 headers (as is common on Linux)
(b) "objectively correct" regardless of compiler version. The code was previously making unreliable assumptions about transitive dependencies within the standard library instead of including the headers it actually needed.

@glennpj
Copy link
Copy Markdown
Contributor

glennpj commented May 17, 2022

The patch in this PR is not sufficient for building bazel versions 4.1 to 4.2.

@adamjstewart
Copy link
Copy Markdown
Member

Closing in favor of #28548 which includes these patches and more.

@elfprince13 elfprince13 reopened this Sep 19, 2023
@elfprince13
Copy link
Copy Markdown
Contributor Author

@adamjstewart - After finally getting around to updating our Spack to 0.20.1, I can confirm that the patches introduced in #28548 do not resolve the issue with ijar assuming a transitive include of <limits>.

@elfprince13
Copy link
Copy Markdown
Contributor Author

I believe the problem is that those patches should be unconditionally applied without checking GCC version because the existing code is demonstrably wrong, and other compilers (e.g. Clang) may use GCC system headers without identifying as GCC.

@elfprince13
Copy link
Copy Markdown
Contributor Author

Closing in favor of #40084.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants