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

Libraryization #156

Merged
merged 6 commits into from
Oct 7, 2019
Merged

Libraryization #156

merged 6 commits into from
Oct 7, 2019

Conversation

thrimbor
Copy link
Member

This splits out PDCLib, hal, Freetype and SDL_ttf to be built as libraries (others are untouched to avoid getting too many merge conflicts).

I am well aware that this is still far from being a complete solution (everything is built with the same compiler flags, libs are automatically added by makefiles instead of the samples specifying their requirements, still the one large main Makefile, sub-Makefiles adding cleaning rules to the main Makefile via a variable etc.) - I consider this only a first step and, most importantly, a fix to get nxdk to build on msys2 again.

@dracc
Copy link
Contributor

dracc commented Sep 23, 2019

NevolutionX.xbe shrunk by 0.2MiB, no files were added to the .iso - did I miss something or does this do magic?

I also noted that I needed to add a link to llvm-lib-8 named llvm-lib in my $PATH on Debian testing (Bullseye) - might be worth adding a note about that somewhere.

@thrimbor
Copy link
Member Author

(This was mostly answered in Discord already by others, but I'll still reply here for completeness)

NevolutionX.xbe shrunk by 0.2MiB, no files were added to the .iso - did I miss something or does this do magic?

This is expected, and one of the reasons behind this change. Atm the linker just links together all object files we pass to it, resulting in lots of dead code ending up in the binary. When using static libraries, lld is able to grab only the required object files from the libraries, resulting in the size reduction. When we switch all of nxdk to be built as libraries, I expect even higher gains.

I also noted that I needed to add a link to llvm-lib-8 named llvm-lib in my $PATH on Debian testing (Bullseye) - might be worth adding a note about that somewhere.

I assume this is an issue of your setup - I didn't test on Debian, but the CI ran it on Ubuntu, macOS and Windows, where it worked fine.

@dracc
Copy link
Contributor

dracc commented Sep 24, 2019

Sorry, I forgot for a moment that Debian is not one of this projects supported OSs, please disregard my comment about llvm-lib.

Anyhow, NevolutionX worked as expected with these changes - nice job!

Copy link
Contributor

@dracc dracc left a comment

Choose a reason for hiding this comment

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

Builds fine, runs fine and the makefile changes look sane.

@thrimbor
Copy link
Member Author

Sorry, I forgot for a moment that Debian is not one of this projects supported OSs, please disregard my comment about llvm-lib.

@dracc I'm certainly interested in having nxdk build properly on Debian. What I meant was that I was suspecting it to be an issue of your specific setup - I have since tried the Debian Docker images though, and found out that it requires you to install the "llvm" package (in addition to clang and lld) to work. I added Debian-specific instructions to the Wiki.

Interestingly, this doesn't seem to be an issue on Ubuntu - CI just installs the llvm package as a dependency.

lib/hal/Makefile Outdated Show resolved Hide resolved
@JayFoxRox
Copy link
Member

This has a conflict now.

@mborgerson
Copy link
Member

Which version of Ubuntu are you testing with? llvm-lib symlink doesn't exist for me on Ubuntu 18.04.

@thrimbor
Copy link
Member Author

thrimbor commented Oct 1, 2019

Which version of Ubuntu are you testing with? llvm-lib symlink doesn't exist for me on Ubuntu 18.04.

I've only tested Ubuntu via Travis CI, that should be 16.04. Do you have the package "llvm" installed (clang and lld don't seem to pull that in on Debian)?

@@ -0,0 +1,18 @@
PDCLIB_SRCS := \
Copy link
Member

@mborgerson mborgerson Oct 1, 2019

Choose a reason for hiding this comment

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

Why isn't this in pdclib repository? It's ok to have here.

@mborgerson
Copy link
Member

@thrimbor

I've only tested Ubuntu via Travis CI, that should be 16.04

Why are you testing with such an old version? You can add a line in your config file to request that a more recent version be used.

Do you have the package "llvm" installed

Ya:

$ dpkg -l | grep llvm
ii  libllvm4.0:amd64                              1:4.0.1-10                                   amd64        Modular compiler and toolchain technologies, runtime library
ii  libllvm6.0:amd64                              1:6.0-1ubuntu2                               amd64        Modular compiler and toolchain technologies, runtime library
ii  libllvm7:amd64                                1:7-3~ubuntu0.18.04.1                        amd64        Modular compiler and toolchain technologies, runtime library
ii  libllvm7:i386                                 1:7-3~ubuntu0.18.04.1                        i386         Modular compiler and toolchain technologies, runtime library
ii  libllvm8:amd64                                1:8.0.1~svn369350-1~exp1~20190820121219.79   amd64        Modular compiler and toolchain technologies, runtime library
ii  llvm                                          1:6.0-41~exp5~ubuntu1                        amd64        Low-Level Virtual Machine (LLVM)
ii  llvm-4.0                                      1:4.0.1-10                                   amd64        Modular compiler and toolchain technologies
ii  llvm-4.0-dev                                  1:4.0.1-10                                   amd64        Modular compiler and toolchain technologies, libraries and headers
ii  llvm-4.0-runtime                              1:4.0.1-10                                   amd64        Modular compiler and toolchain technologies, IR interpreter
ii  llvm-6.0                                      1:6.0-1ubuntu2                               amd64        Modular compiler and toolchain technologies
ii  llvm-6.0-dev                                  1:6.0-1ubuntu2                               amd64        Modular compiler and toolchain technologies, libraries and headers
ii  llvm-6.0-runtime                              1:6.0-1ubuntu2                               amd64        Modular compiler and toolchain technologies, IR interpreter
ii  llvm-7                                        1:7-3~ubuntu0.18.04.1                        amd64        Modular compiler and toolchain technologies
ii  llvm-7-dev                                    1:7-3~ubuntu0.18.04.1                        amd64        Modular compiler and toolchain technologies, libraries and headers
ii  llvm-7-runtime                                1:7-3~ubuntu0.18.04.1                        amd64        Modular compiler and toolchain technologies, IR interpreter
ii  llvm-8                                        1:8.0.1~svn369350-1~exp1~20190820121219.79   amd64        Modular compiler and toolchain technologies
ii  llvm-8-dev                                    1:8.0.1~svn369350-1~exp1~20190820121219.79   amd64        Modular compiler and toolchain technologies, libraries and headers
ii  llvm-8-runtime                                1:8.0.1~svn369350-1~exp1~20190820121219.79   amd64        Modular compiler and toolchain technologies, IR interpreter
ii  llvm-runtime                                  1:6.0-41~exp5~ubuntu1                        amd64        Low-Level Virtual Machine (LLVM), bytecode interpreter

@thrimbor
Copy link
Member Author

thrimbor commented Oct 1, 2019

I've only tested Ubuntu via Travis CI, that should be 16.04

Why are you testing with such an old version? You can add a line in your config file to request that a more recent version be used.

It's just the version nxdk currently uses. If I remember correctly the newer images weren't available when we first added CI.

I looked into the issue in a Ubuntu Docker image, and it's a bug in Ubuntu's llvm-package, which I reported upstream. As a temporary workaround, PATH=$PATH:/usr/lib/llvm-6.0/bin does the trick.

@JayFoxRox
Copy link
Member

JayFoxRox commented Oct 4, 2019

I looked into the issue in a Ubuntu Docker image, and it's a bug in Ubuntu's llvm-package, which I reported upstream. As a temporary workaround, PATH=$PATH:/usr/lib/llvm-6.0/bin does the trick.

Upstream contributor apparently has no intention of backporting a fix for this themselves. I also contacted doko (who uploaded the current revision) on freenode and they responded only with an URL (no context): "https://wiki.ubuntu.com/StableReleaseUpdates" - I believe they imply it does not qualify or we should contact the respective SRU people.

We should have an issue about this after merge.
A possible workaround should be added to the wiki after merge.


I think this is fine and it should be merged; I have tested this and reviewed the code.

@LukeUsher
Copy link
Contributor

Nice work, I think we can merge this as-is.

@thrimbor are you planniing on doing this for the rest of the libraries as a seperate PR?

@thrimbor
Copy link
Member Author

thrimbor commented Oct 7, 2019

@LukeUsher Yes, I plan to have all libraries actually build as libraries eventually. Some are more complicated to adjust though (like SDL, which has its Makefile in the submodule, so it requires an update to the submodule and nxdk before it can work) or were/are undergoing larger changes currently, so this PR is intentionally "incomplete" to avoid problems.

@thrimbor thrimbor deleted the libraryization branch October 12, 2019 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants