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

Add liblzma port #12990

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add liblzma port #12990

wants to merge 1 commit into from

Conversation

Milek7
Copy link
Contributor

@Milek7 Milek7 commented Dec 8, 2020

No description provided.

@welcome
Copy link

welcome bot commented Dec 8, 2020

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @Milek7 !

In general I think this is good.

@sbc100 were you working on some changes to how ports work that could conflict with this? I seem to remember a plan to remove the settings.js changes.

shutil.rmtree(dest_path, ignore_errors=True)
shutil.copytree(source_path, dest_path)

config_args = ['./configure', '--disable-shared']
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to avoid configure, as that may not work on windows. Is there another build system there, or maybe we can run configure and bundle the output config.h or such?

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'll check if this works on Windows. I'd rather avoid pulling the guts from build system if possible. It is ugly and harder to maintain.

Copy link
Contributor Author

@Milek7 Milek7 Dec 10, 2020

Choose a reason for hiding this comment

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

Ok, obviously it won't work on Windows without MinGW shell. There's also cmake available, but if we want building on Windows without other dependencies that would miss the point.

I changed it to don't use any build system, by bundling necessary config defines.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 9, 2020

Yes, I've been trying to push back on adding more ports until I can land the "contrib" ports system I've been working on.

Its still very much WIP: #13002

Since I've not been prioritizing it I feel I have less sway in blocking these kind of changes in the mean time. But, is there some way we could hold of landing this for a few a while? i.e. maybe you can maintain a downstream patch for this for a bit?

We could always land it and then transition it over to contrib I guess? but exactly how contrib ports will work, and how they will get built (for example in emsdk) is still not clear so anyone who started using it right away could get broken by a future move to contrib.

@Milek7 Milek7 force-pushed the liblzma_port branch 5 times, most recently from 212566d to b84c132 Compare December 10, 2020 21:10
@Milek7
Copy link
Contributor Author

Milek7 commented Dec 10, 2020

Well, it's nothing critical, but it would make building fully compatible OpenTTD client on emscripten easier. I think liblzma is used widely enough so it might be useful for others (and other compressors like zlib or bzip2 are already in ports). If not, I think there should be README in ports directory saying that no more ports are accepted for now.

I'm slightly confused how to handle clear() function. Different libname is now generated depending whether settings.USE_PTHREADS is used, as is done in other ports. However, in clear() plain libname is always used (without possible -mt suffix). Is this correct code?

@sbc100
Copy link
Collaborator

sbc100 commented Dec 10, 2020

Well, it's nothing critical, but it would make building fully compatible OpenTTD client on emscripten easier. I think liblzma is used widely enough so it might be useful for others (and other compressors like zlib or bzip2 are already in ports). If not, I think there should be README in ports directory saying that no more ports are accepted for now.

I'm slightly confused how to handle clear() function. Different libname is now generated depending whether settings.USE_PTHREADS is used, as is done in other ports. However, in clear() plain libname is always used (without possible -mt suffix). Is this correct code?

Most ports don't change their libname based on settings so don't need to worry about that.

However of those that do it looks like only regal is doing the right thing. sdl2 and harfbuzz seems to not be honoring the settings during clear().

It also looks like clear() only has a single call site, which we should perhaps consider just removing so we could then just remove all of these clear() functions.

For now, if you want a separate USE_PTHREADS you should copy what regal does I think.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 10, 2020

(do you need a separate pthreads build here?)

@Milek7
Copy link
Contributor Author

Milek7 commented Dec 10, 2020

With pthreads disabled it will be built without pthread dependency, and I think this is desired? Or it is OK to allow it to link to pthread stub, and let it decide in runtime that no extra cores are available?

@Milek7 Milek7 force-pushed the liblzma_port branch 4 times, most recently from be34b1e to 4579a3f Compare December 10, 2020 22:46
@sbc100
Copy link
Collaborator

sbc100 commented Dec 10, 2020

You can assume pthread stubs are always available if it the code purely uses pthreads it maybe OK to have a single build. The main issue would be if there is code in there that uses atomic (C11 or C++11 atomics or GNU builtins, etc).

Base automatically changed from master to main March 8, 2021 23:49
@pelya
Copy link

pelya commented May 1, 2021

I've updated the build script to work in Emscripten 2.0.15

https://github.com/pelya/openttd-android/blob/1.11/os/emscripten/emsdk-liblzma.patch

@pelya
Copy link

pelya commented Dec 10, 2021

The patch updated for Emscripten 2.0.31:
https://github.com/OpenTTD/OpenTTD/blob/master/os/emscripten/emsdk-liblzma.patch

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.

4 participants