-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Allow different cflags depending on extensions in system_libs.py #26037
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
Conversation
We cannot give different cflags depending on the file extensions in the current `system_libs.py`. This can be necessary when a library contains files with multiple extesnsions and some flags only work on a single extension. For example, `-std=c23` only works on .c files, while `-std=c++23` only on .cpp files. This adds a `filename` argument to the system library builder, so that we later can allow different cflags for different extensions.
Yes, I think this approach is probably better so we match upstream flags. |
sbc100
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the filename argument need to be optional?
tools/system_libs.py
Outdated
| cmd += get_base_cflags(self.build_dir, preprocess=False) | ||
| else: | ||
| cmd += cflags | ||
| cmd += self.get_cflags(src) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we already have the extension calculated here perhaps we can add self.cxxflags here when we have a cxx extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understand... We don't have self.cflags either. Do you want to add both? But then why would we have both self.cflags and self.get_cflags() then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have self.cflags and the default self.get_cflags() will read from this field.
The idea is that for simple libraries where the cflags don't vary you can just do cflags = ['x', 'y', 'z'] at the class level.
See libunwind for example.
In this case we could just add support for cxxflags = ['x', 'y', 'z']?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried adding get_cxxflags method here: b482af8
Currently get_cxxflags is set to call get_cflags by default and you can override it.
This doesn't add self.cxxflags because I think then we need to duplicate functions like this for cxxflags:
emscripten/tools/system_libs.py
Lines 589 to 601 in 69334c6
| def get_cflags(self): | |
| """ | |
| Returns the list of flags to pass to emcc when building this variation | |
| of the library. | |
| Override and add any flags as needed to handle new variations. | |
| """ | |
| cflags = self._inherit_list('cflags') | |
| cflags += get_base_cflags(self.build_dir, force_object_files=self.force_object_files) | |
| if self.includes: | |
| cflags += ['-I' + utils.path_from_root(i) for i in self._inherit_list('includes')] | |
| return cflags |
No I don't think so. Changed to a normal argument. |
This adds a test that we compile .c and .cpp files in libunwind with different cflags: .c with `-std=c23`, .cpp with `-std=c++23`.
| return self._get_common_flags() + ['-std=c23'] | ||
|
|
||
| def get_cxxflags(self): | ||
| return self._get_common_flags() + ['-std=c++23'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just looking at libunwin upstream and it seems they do not use c23/c++23, but C++17:
Do I don't think we want to do this maybe? Since we don't want to differ from upstream here.
sbc100
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, although I don't know if we actually want to do this for libunwind in particular.
|
I proposed a revert of llvm/llvm-project#125412. In the mean time can we instead use |
I think we add |
Yeah that's what I originally did... 🙃🫠😞 |
We cannot give different cflags depending on the file extensions in the current
system_libs.py. This can be necessary when a library contains files with multiple extesnsions and some flags only work on a single extension. For example,-std=c23only works on .c files, while-std=c++23only on .cpp files.This adds a
filenameargument to the system library builder, so that we later can allow different cflags for different extensions. This also handlesUSE_NINJApath.A test for libunwind is added that we compile .c files with
-std=c23and .cpp files with-std=c++23. (This we want to do for LLVM 21 anyway)