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

Initial VCPkg integration #4226

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Initial VCPkg integration #4226

wants to merge 27 commits into from

Conversation

jediry
Copy link

@jediry jediry commented Sep 11, 2022

This change introduces the VCPkg builder integrating the vcpkg system (http://vcpkg.io) for acquiring and building 3rd party C/C++ libraries. This enables an SConstruct file to say, for example, "env.VCPkg('openjpeg')", and then vcpkg will handle downloading the source code and building it. The VCPkg() builder further ensures that the vcpkg include directory gets added to CPPPATH, and that its static lib directory gets added to LIBPATH.

One notable quirk of the VCPkg builder is that it does the actual package build during the initial execution of the SConstruct file, rather than occurring later, during SCons's normal build phase. While undesirable, this ensures that the VCPkg builder is able to accurately compute the set of files produced by building the package (as of this writing, vcpkg does not support any means to predict the files generated by building a package). I hope to improve this in the future.

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt (and read the README.rst)
  • I have updated the appropriate documentation

<tool name="vcpkg">
<summary>
<para>
Sets construction variables for the &vcpkg; package manager
Copy link
Collaborator

@mwichmann mwichmann Sep 11, 2022

Choose a reason for hiding this comment

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

The generated entities for a tool named vcpkg will be &t-vcpkg; and &t-link-vcpgk; - thus this entity reference generated an error.

 ERROR: SCons/Tool/vcpkg.xml fails to parse:
Entity 'vcpkg' not defined, line 29, column 60 (vcpkg.xml, line 29)

@mwichmann mwichmann added enhancement Tools Issues related to tools subsystem labels Sep 11, 2022
@@ -0,0 +1,528 @@
#
# __COPYRIGHT__
Copy link
Collaborator

Choose a reason for hiding this comment

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

For new files, please follow the templates in templates/file.py and templates/test.py - some files have not yet been converted, in particular Tool modules, so realize you probably got this form from an existing file...

@@ -809,6 +809,8 @@ def tool_list(platform, env):
'tar', 'zip',
# File builders (text)
'textfile',
# Package management
'vcpkg',
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this should be a win32 only thing? Or does it work on all platforms?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like win, linux, and macos. So it shouldn't be in default list. (BSD's not supported?, hpux, solaris, aix, other evil remnants of the past which are still in use(that's not referring to BSDs, but rather the others.. ;)

Copy link
Contributor

@bdbaddog bdbaddog Sep 11, 2022

Choose a reason for hiding this comment

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

further review looks like your implementation is win32 only, so should be only in that platforms default tool list above. under other_plat_tools

Copy link
Author

@jediry jediry Sep 11, 2022

Choose a reason for hiding this comment

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

My implementation is not Win32-only. I've tested on Ubuntu and Windows, and it should work on Mac as well, though I don't have the ability to test Mac, as I don't own one.

OK, so if I'm understanding correctly, I should conditionally add this to other_plat_tools on platforms where it's supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. add it to other_plat_tools for each supported platform. Also I'd add a note which platforms it's supported on in the docs and CHANGES/RELEASE files.

Copy link
Author

Choose a reason for hiding this comment

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

Ummm...how do I detect "linux"? It seems like tools such as m4 and rpm are being added simply based on "not win32". Is there a positive test I can do to detect a linux system?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ummm...how do I detect "linux"? It seems like tools such as m4 and rpm are being added simply based on "not win32". Is there a positive test I can do to detect a linux system?

Eh, that's a little complex. Depending on where you're asking... a simple way is to check sys.platform for linux. However, the name of the Platform module is actually posix, which is shared with some of the other choices (that originally comes from the value of os.name).

CHANGES.txt Outdated Show resolved Hide resolved
@mwichmann
Copy link
Collaborator

Adding a reference to #4101 for tracking purposes.

@jediry
Copy link
Author

jediry commented Sep 11, 2022

@mwichmann @bdbaddog what are the expectations regarding code coverage? If I'm reading this correctly, my unit tests only exercise 41% of the lines of code. There are certainly more tests that could be written (I've left a TODO in the test file listing scenarios that I think would be reasonable to test), but 100% code coverage is not feasible.

@bdbaddog
Copy link
Contributor

@mwichmann @bdbaddog what are the expectations regarding code coverage? If I'm reading this correctly, my unit tests only exercise 41% of the lines of code. There are certainly more tests that could be written (I've left a TODO in the test file listing scenarios that I think would be reasonable to test), but 100% code coverage is not feasible.

We don't have hard requirements on code coverage. That said if you can reasonable add more tests to cover the major code branches, please do.

"""Returns the list of C/C++ header files belonging to the package.
If `transitive` is True, then files belonging to upstream dependencies of this package are also included."""
_vcpkg_print(Debug, str(self.descriptor) + ': headers')
files = self.FilesUnderSubPath('include/', transitive)
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Flake8] local variable 'files' is assigned to but never used (view)

Rule
F841

You can close this issue if no need to fix it. Learn more.

Looks like duplicated work, maybe just drop this line?

self.files = _read_vcpkg_file_list(self.env, self)
self.loaded = True

triplet = self.descriptor.get_triplet()
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Flake8] local variable 'triplet' is assigned to but never used (view)

Rule
F841

You can close this issue if no need to fix it. Learn more.

get_triplet called, then called again. maybe delete this line, or use triplet in the next?

@bdbaddog
Copy link
Contributor

bdbaddog commented Sep 12, 2022

Probably worth addressing all the sider complaints.

# State modification functions used by contextmanager functions below
#

def addAvailablePackage(self, name, version, dependencies):
Copy link
Author

Choose a reason for hiding this comment

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

[Flake8] expected 1 blank line, found 0 (view)

Rule
E301

You can close this issue if no need to fix it. Learn more.

I don't understand this flake8 complaint. Is it asking me to put a blank line after "def addAvailablePackage(...):"?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you google "flake8 e301" (The error indicated), you'll usually find a decent explanation.
see: https://www.flake8rules.com/rules/E301.html (for this case)

Copy link
Author

Choose a reason for hiding this comment

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

I did find that page...but it was unhelpful in this case. As you can see, there is a blank line before "def addAvailablePackage" and a blank line after the end of the function. So I don't know what to do to appease this rule...

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can mark it to skip, I don't see anything there to complain about

Copy link
Contributor

Choose a reason for hiding this comment

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

I closed a couple of the sider issues. The remaining ones are suggesting using def instead of lambda. @mwichmann - thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't personally have a strong opinion. The thinking these days seems to be if you want to use an anonymous function do it; if you're going to assign a name to it then use a real function instead - that's pretty much what the error message says anyway. lambdas work well for things like sort keys, so they definitely still have a place, it's just a smaller place than it used to be.

Copy link
Author

Choose a reason for hiding this comment

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

My inclination is that using "def" here is the wrong thing. I'm not just using the lambda to avoid declaring an extra function. I'm also using the fact that lambdas can capture state from the surrounding context (in this case, the suffix_filters variable). I would vote to leave this as-is.

If you feel strongly, the other options I see are:

  1. Convert this into a single def, containing the entire "if...else" block on lines 527-532. Slightly less performant in the case where the user wants to enumerate all files without a filter, but that's probably not the common case.
  2. Convert this into 3 defs (one for each case), assign the appropriate one to a local var, and then pass the suffix_filters variable as an argument when invoking. Feels clunky to me.

Let me know what you prefer.

@mwichmann
Copy link
Collaborator

Sigh. The doc build failure is one I started to run into as well. I was able to stave it off by backing out an example - that was the thing that triggered it, but there was nothing wrong with my snippets, it just someone was one piece too much for lxml (see #4235). In other words, it's going to be tricky to figure out what's going wrong here and in that PR.

@jediry
Copy link
Author

jediry commented Sep 25, 2022

Bummer. Also, on a related note, I noticed that the SconsDoc stuff tries to load lxml, and if that fails, tries to fall back to a different implementation, but that implementation apparently doesn't support XSD schema validation? In any case, it falls down pretty hard, with no clue on how to fix it. I finally figured out I needed to "pip install lxml". IMO, we should rip out the fallback XML parser and instead have a friendly failure message to the person running SconsDoc that they need to install lxml. I'll log an issue on this.

@jediry
Copy link
Author

jediry commented Sep 25, 2022

Bummer. Also, on a related note, I noticed that the SconsDoc stuff tries to load lxml, and if that fails, tries to fall back to a different implementation, but that implementation apparently doesn't support XSD schema validation? In any case, it falls down pretty hard, with no clue on how to fix it. I finally figured out I needed to "pip install lxml". IMO, we should rip out the fallback XML parser and instead have a friendly failure message to the person running SconsDoc that they need to install lxml. I'll log an issue on this.

The "new issue" text tells me to raise this on Discord or the mailing list first. Should I log this issue?

@bdbaddog
Copy link
Contributor

Bummer. Also, on a related note, I noticed that the SconsDoc stuff tries to load lxml, and if that fails, tries to fall back to a different implementation, but that implementation apparently doesn't support XSD schema validation? In any case, it falls down pretty hard, with no clue on how to fix it. I finally figured out I needed to "pip install lxml". IMO, we should rip out the fallback XML parser and instead have a friendly failure message to the person running SconsDoc that they need to install lxml. I'll log an issue on this.

The "new issue" text tells me to raise this on Discord or the mailing list first. Should I log this issue?

Please bring it to the users mailing list or the discord server.. :)

@mwichmann
Copy link
Collaborator

So, yes, there was a transitional phase... lxml was the "second choice" in the Python 2 days, but the other approach hasn't been supported for a while, and doesn't wotk on Py3. Not everything relating to "the old days" has been ripped out completely, just because... we're (somewhat) human, I guess. There's a script to set up for development (bin/scons_dev_master.py) which is documented somewhere, and which also has an error in it, so it will blow up after installing the deps - just found that the other day. Meanwhile, in the current source tree (not in any released version) there are now three requirements files: the base one for running scons, requirements-dev.txt which adds the stuff you need basically to run the full test suite, and requirements-pkg.txt, which is "everything" - basically adding the stuff needed to build the docs. lxml is in the requirements-dev.txt since it's used by some of the docbook and xml tests.

@mwichmann
Copy link
Collaborator

Oh - and scons_dev_master is only set up for Debian-ish systems.

@jediry
Copy link
Author

jediry commented Oct 11, 2022 via email

@jediry
Copy link
Author

jediry commented Dec 15, 2022

Sigh. The doc build failure is one I started to run into as well. I was able to stave it off by backing out an example - that was the thing that triggered it, but there was nothing wrong with my snippets, it just someone was one piece too much for lxml (see #4235). In other words, it's going to be tricky to figure out what's going wrong here and in that PR.

Hi @mwichmann . I'm finally getting back to this project. Since the doc generation is blocking the PR, I think I might take a stab at fixing that problem. Have you done any investigation on this issue yet? My suspicion is that this might be an out-of-memory issue, either in Python or in FOP (a Java tool, IIRC). I'm wondering if it might be as simple as allocating more "max" heap memory to the JVM that runs FOP. Also, have you been able to get a local repro for this issue? I tried to repro it myself on my Ubuntu WSL virtual machine back in October, but as I recall, I was not able to hit this, at least not on the few attempts I've made.

@mwichmann
Copy link
Collaborator

The doc build part appears to be a built-in limit that turns out to now be tunable thanks to someone's efforts about six months ago. I bumped the limit again for #4263, as those changes pushed me into the "trouble" area, but that has not been merged yet. Whether it triggers does seem to be situational. In any case, if you want, you can try the same bit:

diff --git a/SCons/Tool/docbook/__init__.py b/SCons/Tool/docbook/__init__.py
index 5cf5e6198..52e291144 100644
--- a/SCons/Tool/docbook/__init__.py
+++ b/SCons/Tool/docbook/__init__.py
@@ -69,7 +69,7 @@ re_refname = re.compile(r"<refname>([^<]*)</refname>")
 # lxml etree XSLT global max traversal depth
 #
 
-lmxl_xslt_global_max_depth = 3100
+lmxl_xslt_global_max_depth = 3600
 
 if has_lxml and lmxl_xslt_global_max_depth:
     def __lxml_xslt_set_global_max_depth(max_depth):

@mwichmann
Copy link
Collaborator

Bummer. Also, on a related note, I noticed that the SconsDoc stuff tries to load lxml, and if that fails, tries to fall back to a different implementation, but that implementation apparently doesn't support XSD schema validation? In any case, it falls down pretty hard, with no clue on how to fix it. I finally figured out I needed to "pip install lxml". IMO, we should rip out the fallback XML parser and instead have a friendly failure message to the person running SconsDoc that they need to install lxml. I'll log an issue on this.

FWIW, there's now a straightforward way to get all the deps in place if you intend to "build scons", which includes building the doc set:

pip install -r requirements-pkg.txt

@jediry
Copy link
Author

jediry commented Jan 16, 2023

The doc build part appears to be a built-in limit that turns out to now be tunable thanks to someone's efforts about six months ago. I bumped the limit again for #4263, as those changes pushed me into the "trouble" area, but that has not been merged yet. Whether it triggers does seem to be situational. In any case, if you want, you can try the same bit:

diff --git a/SCons/Tool/docbook/__init__.py b/SCons/Tool/docbook/__init__.py
index 5cf5e6198..52e291144 100644
--- a/SCons/Tool/docbook/__init__.py
+++ b/SCons/Tool/docbook/__init__.py
@@ -69,7 +69,7 @@ re_refname = re.compile(r"<refname>([^<]*)</refname>")
 # lxml etree XSLT global max traversal depth
 #
 
-lmxl_xslt_global_max_depth = 3100
+lmxl_xslt_global_max_depth = 3600
 
 if has_lxml and lmxl_xslt_global_max_depth:
     def __lxml_xslt_set_global_max_depth(max_depth):

OK! I've added this change (I upped it to 4000 for good measure), and that seems to have fixed the issue for me. Thank you!

@bdbaddog
Copy link
Contributor

bdbaddog commented Oct 4, 2023

@jediry - Sorry for the delay. Finally got a big chunk of time to review this and read up on vcpkg and (hopefully) better understand.

Let me ask you a fundamental question about the goal of this functionality.
Is it really:

  1. Check and see if this package is already installed, and if not install it and configure scons to be able to use it's libraries and header files (and binaries)?
  2. Build this package as part of my build?

I'm thinking it's really #1?

@jediry
Copy link
Author

jediry commented Oct 4, 2023

I'm thinking it's really #1?

Correct. Though, if the specified package is not currently installed, its binaries will be built as part of the install process, so #2 kind of applies as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Tools Issues related to tools subsystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants