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

Statically build and link the ANTLR4 runtime. #41

Merged
merged 12 commits into from
Dec 17, 2020

Conversation

HackerFoo
Copy link
Contributor

Statically link the ANTLR4 runtime to remove the runtime dependency. The source build will automatically pull necessary code from GitHub.

$ ldd build/lib.linux-x86_64-3.7/fasm/parser/libparse_fasm.so
	linux-vdso.so.1 (0x00007fff4cfb6000)
	libstdc++.so.6 => /nix/store/danv012gh0aakh8xnk2b35vahklz72mk-gcc-9.2.0-lib/lib/libstdc++.so.6 (0x00007f69947fb000)
	libm.so.6 => /nix/store/xg6ilb9g9zhi2zg1dpi4zcp288rhnvns-glibc-2.30/lib/libm.so.6 (0x00007f69946bb000)
	libgcc_s.so.1 => /nix/store/xg6ilb9g9zhi2zg1dpi4zcp288rhnvns-glibc-2.30/lib/libgcc_s.so.1 (0x00007f69946a1000)
	libc.so.6 => /nix/store/xg6ilb9g9zhi2zg1dpi4zcp288rhnvns-glibc-2.30/lib/libc.so.6 (0x00007f69944e2000)
	/nix/store/xg6ilb9g9zhi2zg1dpi4zcp288rhnvns-glibc-2.30/lib64/ld-linux-x86-64.so.2 (0x00007f6994b0e000)

@HackerFoo HackerFoo force-pushed the static_antlr4_runtime branch 3 times, most recently from b670a21 to 124fa7c Compare December 15, 2020 20:09
@HackerFoo
Copy link
Contributor Author

This should be a much better solution than f4pga/f4pga-arch-defs#1880

Copy link
Collaborator

@litghost litghost left a comment

Choose a reason for hiding this comment

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

Would it be possible to make whether to compile statically or dynamically an option? I can see a case for both configurations.

@HackerFoo
Copy link
Contributor Author

Would it be possible to make whether to compile statically or dynamically an option? I can see a case for both configurations.

I'm sure it's possible, but it would take time. How useful is this feature? Could it be done in a later PR?

@litghost
Copy link
Collaborator

Would it be possible to make whether to compile statically or dynamically an option? I can see a case for both configurations.

I'm sure it's possible, but it would take time. How useful is this feature?

We've already gotten requests in prjxray to enable unvendored dependencies, see f4pga/prjxray#1376 . While I'm never excited about adding work for packaging, I'm also unexcited about knowingly creating work for the future when it can be avoided.

Could it be done in a later PR?

I don't believe so.

@HackerFoo
Copy link
Contributor Author

What would be the advantage of being able to select a shared library in this specific case?

Would it be possible to make whether to compile statically or dynamically an option? I can see a case for both configurations.

I'm sure it's possible, but it would take time. How useful is this feature?

We've already gotten requests in prjxray to enable unvendored dependencies, see SymbiFlow/prjxray#1376 . While I'm never excited about adding work for packaging, I'm also unexcited about knowingly creating work for the future when it can be avoided.

Could it be done in a later PR?

I don't believe so.

What would be the advantage of being able to select a shared library in this specific case? The ANTLR runtime is not a large dependency. Note that this doesn't include the ANTLR parser generator itself, which is only used at compile time.

@HackerFoo HackerFoo force-pushed the static_antlr4_runtime branch 2 times, most recently from 970fac1 to cc684d0 Compare December 16, 2020 02:55
@HackerFoo
Copy link
Contributor Author

I have added a flag to use an external shared library for the ANTLR runtime:

$ python setup.py build_ext --help
Common commands: (see '--help-commands' for more)
...

Options for 'CMakeBuild' command:
  --antlr-runtime  Whether to use a 'static' or 'shared' ANTLR runtime.
  --help-compiler  list available compilers

...

--antlr-runtime=static is the default, and --antlr-runtime=shared will look for libantlr-runtime using CMake's find_library

setup.py Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@litghost litghost left a comment

Choose a reason for hiding this comment

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

Can GH actions add a matrix option on static/dynamic easily?

@HackerFoo
Copy link
Contributor Author

@litghost

  • I've removed the modified CMake files and added an unmodified submodule at third_party/antlr4. This avoids any maintenance of upstream code and possible divergence.
  • I've added a flag to setup.py to allow using the system provided shared library for the ANTLR4 runtime, which will work better with modern OSes which provide this library.

Copy link
Collaborator

@litghost litghost left a comment

Choose a reason for hiding this comment

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

The current flag approach doesn't appear to work. As a concrete example, how (via pip) can a user install the fasm package using a system dynamic library instead of the static library?

@HackerFoo
Copy link
Contributor Author

The current flag approach doesn't appear to work. As a concrete example, how (via pip) can a user install the fasm package using a system dynamic library instead of the static library?

I have added this flag to all the relevant commands, and documentation on how to use this with pip in the README.

@litghost
Copy link
Collaborator

litghost commented Dec 17, 2020

The current flag approach doesn't appear to work. As a concrete example, how (via pip) can a user install the fasm package using a system dynamic library instead of the static library?

I have added this flag to all the relevant commands, and documentation on how to use this with pip in the README.

I don't believe this works? Maybe I'm missing something?

python3 -mvenv test_pip
source test_pip/bin/activate
pip install cython textx
pip install -v git+https://github.com/HackerFoo/fasm.git@static_antlr4_runtime --install-option="--antlr-runtime=shared"
ldd test_pip/lib/python3.8/site-packages/fasm//parser/libparse_fasm.so 

Outputs:

$ ldd /usr/local/google/home/keithrothman/cat_x/test_pip/lib/python3.8/site-packages/fasm//parser/libparse_fasm.so 
        linux-vdso.so.1 (0x00007ffd0d976000)
        libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f590dad9000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f590d995000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f590d97b000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f590d7b6000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f590ddfe000)

For clarity, the following does work:

python3 setup.py build_ext --antlr-runtime=shared
ldd ./build/lib.linux-x86_64-3.8/fasm/parser/libparse_fasm.so

outputs:

$ ldd ./build/lib.linux-x86_64-3.8/fasm/parser/libparse_fasm.so
        linux-vdso.so.1 (0x00007ffebdff4000)
        libantlr4-runtime.so.4.8 => /usr/lib/x86_64-linux-gnu/libantlr4-runtime.so.4.8 (0x00007f9f72e6c000)
        libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f9f72c9f000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f9f72b5b000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f9f72b41000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f9f7297c000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f9f72ff4000)

@litghost
Copy link
Collaborator

litghost commented Dec 17, 2020

FYI, I know that whatever PyYAML is doing works with this: https://github.com/yaml/pyyaml/blob/master/setup.py#L120

PyYAML has an option to use libyaml, with a python fallback (kind of like what we are doing). I debugged PyYAML not using the binary parser with the following invocation:

pip install  --global-option='--with-libyaml' pyyaml
GitHub
Canonical source repository for PyYAML. Contribute to yaml/pyyaml development by creating an account on GitHub.

@HackerFoo
Copy link
Contributor Author

@litghost It looks like a known bug with VCS urls: pypa/pip#5518

@litghost
Copy link
Collaborator

@litghost It looks like a known bug with VCS urls: pypa/pip#5518

Fixed in 2019? pypa/pip#6389 ?

@litghost
Copy link
Collaborator

@litghost It looks like a known bug with VCS urls: pypa/pip#5518

I am testing whether the install from local directory works and the VCS does not.

@HackerFoo
Copy link
Contributor Author

Even when I try random flags like --with-foo=bar, nothing happens, so I'm pretty sure flags are being ignore entirely. I'm trying to figure out why.

@HackerFoo
Copy link
Contributor Author

The flag seems to affect pyyaml's setup.py even when using a Git URL, so that isn't it.

@HackerFoo
Copy link
Contributor Author

The presence of pyproject.toml seems to cause flags to be ignored. It works when I remove the file.

Signed-off-by: Dusty DeWeese <[email protected]>
@HackerFoo
Copy link
Contributor Author

@litghost I found that the --no-use-pep517 flag for pip will allow flags to work as expected. This is needed because the existence of pyproject.toml enables pip's PEP517 mode.

tox.ini Outdated Show resolved Hide resolved
@litghost
Copy link
Collaborator

@litghost I found that the --no-use-pep517 flag for pip will allow flags to work as expected. This is needed because the existence of pyproject.toml enables pip's PEP517 mode.

Weird! Good find.

@HackerFoo HackerFoo force-pushed the static_antlr4_runtime branch 2 times, most recently from fca5006 to 859c618 Compare December 17, 2020 19:51
tox.ini Outdated Show resolved Hide resolved
Signed-off-by: Dusty DeWeese <[email protected]>
Copy link
Collaborator

@litghost litghost left a comment

Choose a reason for hiding this comment

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

LGTM, the --no-pep-517 does appear to fix the problem with the flags. Can you add a comment in both places you mention/use the --no-pep-517 flag with why it is there. Relevant issue: pypa/pip#5771

Signed-off-by: Dusty DeWeese <[email protected]>
@HackerFoo
Copy link
Contributor Author

LGTM, the --no-pep-517 does appear to fix the problem with the flags. Can you add a comment in both places you mention/use the --no-pep-517 flag with why it is there. Relevant issue: pypa/pip#5771

Done, and I've added an issue to update when there is a better way to do this: #44

@litghost litghost merged commit 7124d4f into chipsalliance:master Dec 17, 2020
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.

2 participants