Provide `out_data_dirs` attribute (#419) #622

jheaff1 · 2021-04-27T16:12:27Z

When the python executable, built using configure_make, is invoked it requires the files within the generated lib folder to be available on the file system.

This commit provides an out_data_dirs attribute, which declares the given directories as outputs to the bazel build action. By specifying the generated lib dir as one of the output_data_dirs, the lib directory is accessible when the python executable is invoked.

This facilitates hermetic python toolchains, built using the awesome rules_foreign_cc repo

This resolves #419

UebelAndre

I'm not sure how I feel about accepting directories as the output group here. Given that Bazel can't make virtually any determinism guarantees about these build systems, I think it's better for the rules to request specific outputs. Though, I'd imagine there'd be a massive list of files here if you needed to be explicit. Maybe @jsharpe would have some thoughts there

Also this target doesn't seem to be hooked up. The examples are in a less than ideal state and require you to add things to a test_suite to get them tested:

rules_foreign_cc/examples/third_party/BUILD.bazel

Lines 3 to 59 in 98c88de

    
           test_suite( 
        
               name = "linux_tests", 
        
               tags = ["manual"], 
        
               tests = [ 
        
                   "//bison:bison_build_test", 
        
                   "//cares:test_c_ares", 
        
                   "//curl:curl_build_test", 
        
                   "//gn:gn_launch_test", 
        
                   "//libgit2:libgit2_build_test", 
        
                   "//libpng:test_libpng", 
        
                   "//libssh2:libssh2_build_test", 
        
                   "//openssl:openssl_build_test", 
        
                   "//pcre:pcre_build_test", 
        
               ], 
        
           ) 
        
           test_suite( 
        
               name = "linux_rbe_tests", 
        
               tags = ["manual"], 
        
               tests = [ 
        
                   # Missing a new enough m4 to build 
        
                   #"//bison:bison_build_test", 
        
                   "//cares:test_c_ares", 
        
                   "//curl:curl_build_test", 
        
                   # Attempts to access git sha during configure of build so fails 
        
                   #"//gn:gn_launch_test", 
        
                   "//libgit2:libgit2_build_test", 
        
                   "//libpng:test_libpng", 
        
                   "//libssh2:libssh2_build_test", 
        
                   "//openssl:openssl_build_test", 
        
                   "//pcre:pcre_build_test", 
        
               ], 
        
           ) 
        
           test_suite( 
        
               name = "macos_tests", 
        
               tags = ["manual"], 
        
               tests = [ 
        
                   "//cares:test_c_ares", 
        
                   "//curl:curl_build_test", 
        
                   "//gn:gn_launch_test", 
        
                   "//iconv:iconv_build_test", 
        
                   "//libgit2:libgit2_build_test", 
        
                   "//libpng:test_libpng", 
        
                   "//libssh2:libssh2_build_test", 
        
                   "//openssl:openssl_build_test", 
        
                   "//pcre:pcre_build_test", 
        
               ], 
        
           ) 
        
           test_suite( 
        
               name = "windows_tests", 
        
               tags = ["manual"], 
        
               tests = [ 
        
                   # TODO: Add windows tests 
        
               ], 
        
           )

Can you add the new target there?

examples/third_party/python/BUILD.bazel

examples/third_party/python/BUILD.python.bazel

jheaff1 · 2021-04-27T16:49:20Z

@UebelAndre I have actioned your change requests.

I agree that this is against bazel best practice, although the same can be said for how includes are handled in this repository as they too are added using ctx.actions.declare_directory.

The out_data_dirs attribute is optional and those that choose to utilise it can greatly benefit from the feature (by not having to list all the expected output files of the build), accepting the lack of full determinism

examples/third_party/BUILD.bazel

examples/third_party/python/BUILD.python.bazel

jsharpe · 2021-04-27T19:08:47Z

examples/third_party/python/BUILD.bazel

+
+py_runtime(
+    name = "py3_runtime",
+    files = ["@python"],


Could you instead use the gen_dir output group to create a filegroup with all of these files? If using gen_dir includes too much you can always use the postfix_script attribute to remove any files you don't need from the install directory?
If that works then this would avoid the need to add an extra attribute.

This also seems related to #582

@jsharpe I'll look into using gen_dir

@jsharpe I tried adding the following filegroup to Build.python3.bazel

filegroup( name = "gen_dir", srcs = [":python3"], output_group = "gen_dir", )

I then set the files attribute of the py_runtime to ["@python3//:gen_dir]. Sadly the files inside the lib directory generated by the build of python were not included in the runfiles for the python3_test. As such, the invocation of python3 failed.

Perhaps I am doing something wrong?

examples/WORKSPACE

UebelAndre · 2021-06-17T16:03:47Z

It seems that zlib isn't actually a dependency of Python3. I'm not sure why I thought it was to begin with. I ran ./configure --with-zlib in the python3 repo and a warning appeared stating that --with-zlib is an unrecognised argument. I have now removed the dependency on zlib

Turns out I was correct in the first place. After removing the dependency on zlib, there is the following error:
ZipImportError: can't decompress data; zlib not available
I'll add zlib back as a dependency

Even more confusing then! lol How is it possible to build py3 if the path is mangled? Maybe it's a linux only feature? I pulled your branch earlier and successfully built. I'm otherwise not sure how that's possible.

jheaff1 · 2021-06-17T16:14:59Z

It seems that zlib isn't actually a dependency of Python3. I'm not sure why I thought it was to begin with. I ran ./configure --with-zlib in the python3 repo and a warning appeared stating that --with-zlib is an unrecognised argument. I have now removed the dependency on zlib

Turns out I was correct in the first place. After removing the dependency on zlib, there is the following error:
ZipImportError: can't decompress data; zlib not available
I'll add zlib back as a dependency
Even more confusing then! lol How is it possible to build py3 if the path is mangled? Maybe it's a linux only feature? I pulled your branch earlier and successfully built. I'm otherwise not sure how that's possible.

Yeah it is quite bizarre! Are you testing on Linux or Mac?

jheaff1 · 2021-06-17T16:15:39Z

The latest build (https://buildkite.com/bazel/rules-foreign-cc/builds/2913#7a58e48b-85b0-4b71-8e4f-6899742b121d) has the same error as before but for Python2 now:

cp: symlink: python2: File exists

UebelAndre · 2021-06-17T16:28:19Z

Yeah it is quite bizarre! Are you testing on Linux or Mac?

I'm only testing on my mac.

The latest build (https://buildkite.com/bazel/rules-foreign-cc/builds/2913#7a58e48b-85b0-4b71-8e4f-6899742b121d) has the same error as before but for Python2 now:
cp: symlink: python2: File exists

I'm a bit busy today so I don't have too much time to dig into this but can you narrow down what specific call is failing here? Maybe by wrapping some commands in set -x + set +x?

UebelAndre · 2021-06-17T17:07:11Z

Also worth noting that it's the standalone build which failed again.....

jheaff1 · 2021-06-17T17:32:37Z

The error seems to occur during or after the make install part of the build. The error may occur in the copy_dir_contents_to_dir method

jheaff1 · 2021-06-17T18:05:52Z

I added debug prints to the start and end of copy_dir_contents_to_dir (which is a recursive function) and the build log suggests that it never got to the end of the function, so copy_dir_contents_to_dir must be the culprit

foreign_cc/private/framework/toolchains/macos_commands.bzl

UebelAndre · 2021-06-18T02:55:06Z

Ok, finally, I think #687 will solve the issue....

jheaff1 · 2021-06-18T08:29:08Z

Ok, finally, I think #687 will solve the issue....

Awesome! I have merged your branch into this one to see if the standalone MacOS builds are successful first time

jheaff1 · 2021-06-18T08:43:40Z

Ok, finally, I think #687 will solve the issue....

Awesome! I have merged your branch into this one to see if the standalone MacOS builds are successful first time

It passed! Once PR #687 is merged I will rebase this PR off main

jheaff1 · 2021-06-22T12:21:05Z

@UebelAndre I have rebased off the main branch and resolved the build failures, although the "docs" portion of the build is now failing.

UebelAndre · 2021-06-22T15:10:36Z

@UebelAndre I have rebased off the main branch and resolved the build failures, although the "docs" portion of the build is now failing.

Hey, yeah, I unfortunately had to restore the need for PRs to commit docs. So you'll have to run cd docs && bazel run :generate_docs and commit the results. I'm working on a permanent solution for this but that should fix the docs issue.

I'm still waiting on my reviewer for #687 so once that's in then I think this will immediately follow

jheaff1 · 2021-06-22T15:15:27Z

@UebelAndre I have rebased off the main branch and resolved the build failures, although the "docs" portion of the build is now failing.

Hey, yeah, I unfortunately had to restore the need for PRs to commit docs. So you'll have to run cd docs && bazel run :generate_docs and commit the results. I'm working on a permanent solution for this but that should fix the docs issue.

I'm still waiting on my reviewer for #687 so once that's in then I think this will immediately follow

Great, thanks. I figured the docs out about 10 mins ago and the build is now all green :)

This change facilitates hermetic python toolchains, as demonstrated by the test added in this commit.

jsharpe · 2021-06-22T15:27:13Z

I'm still waiting on my reviewer for #687 so once that's in then I think this will immediately follow

Done and merged.

jheaff1 · 2021-06-22T15:31:46Z

@UebelAndre @jsharpe This PR is now ready for re-review :)

UebelAndre

What an amazing job you've done @jheaff1 !!!!

I'll give @jsharpe a moment to look but I think this is ready to go in

jsharpe · 2021-06-22T15:41:42Z

examples/third_party/python/BUILD.python2.bazel

+        "//conditions:default": [],
+    }),
+    configure_options = [
+        "CFLAGS='-Dredacted=\"redacted\"'",


We need to revisit the quoting if this option is still required.

but at least this will act as a test to make sure its fixed!

jsharpe

LGTM

jheaff1 · 2021-06-22T16:10:58Z

What an amazing job you've done @jheaff1 !!!!

I'll give @jsharpe a moment to look but I think this is ready to go in

Thank you very much indeed! Happy I could contribute to this awesome repo

jheaff1 requested a review from oquenchil as a code owner April 27, 2021 16:12

jheaff1 mentioned this pull request Apr 27, 2021

Support "data" outputs #419

Closed

jheaff1 force-pushed the out_data_dirs branch from 80ae4ef to c8d8978 Compare April 27, 2021 16:14

UebelAndre requested changes Apr 27, 2021

View reviewed changes

jheaff1 force-pushed the out_data_dirs branch 2 times, most recently from 12f53d6 to deb2fae Compare April 27, 2021 16:44

jheaff1 force-pushed the out_data_dirs branch from deb2fae to 945bb52 Compare April 27, 2021 16:55

UebelAndre requested changes Apr 27, 2021

View reviewed changes

examples/third_party/BUILD.bazel Outdated Show resolved Hide resolved

examples/third_party/python/BUILD.python.bazel Outdated Show resolved Hide resolved

jheaff1 force-pushed the out_data_dirs branch 12 times, most recently from 505b81f to 3d2d3fd Compare April 27, 2021 18:12

jsharpe reviewed Apr 27, 2021

View reviewed changes

UebelAndre requested changes Apr 27, 2021

View reviewed changes

examples/WORKSPACE Outdated Show resolved Hide resolved

jheaff1 force-pushed the out_data_dirs branch 7 times, most recently from e5a8a52 to 1a15e14 Compare April 28, 2021 17:11

jheaff1 force-pushed the out_data_dirs branch from 4774623 to a6e0567 Compare June 17, 2021 16:00

jheaff1 force-pushed the out_data_dirs branch from a6e0567 to 6d1c40d Compare June 17, 2021 16:07

jheaff1 force-pushed the out_data_dirs branch from e3ab48c to 6d9bcdd Compare June 17, 2021 17:31

UebelAndre reviewed Jun 17, 2021

View reviewed changes

foreign_cc/private/framework/toolchains/macos_commands.bzl Outdated Show resolved Hide resolved

UebelAndre mentioned this pull request Jun 18, 2021

Updated macos copy_dir_contents_to_dir to be like other platforms #687

Merged

jheaff1 force-pushed the out_data_dirs branch from d5ea189 to f349219 Compare June 22, 2021 12:06

jheaff1 force-pushed the out_data_dirs branch from f349219 to 7d244c5 Compare June 22, 2021 15:05

jheaff1 force-pushed the out_data_dirs branch from 7d244c5 to 8b965f2 Compare June 22, 2021 15:23

Provide out_data_dirs attribute (bazelbuild#419)

8b965f2

This change facilitates hermetic python toolchains, as demonstrated by the test added in this commit.

UebelAndre approved these changes Jun 22, 2021

View reviewed changes

jsharpe reviewed Jun 22, 2021

View reviewed changes

jsharpe approved these changes Jun 22, 2021

View reviewed changes

UebelAndre merged commit 5663d88 into bazelbuild:main Jun 22, 2021

jheaff1 deleted the out_data_dirs branch June 22, 2021 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide `out_data_dirs` attribute (#419) #622

Provide `out_data_dirs` attribute (#419) #622

jheaff1 commented Apr 27, 2021

UebelAndre left a comment

jheaff1 commented Apr 27, 2021 •

edited

Loading

jsharpe Apr 27, 2021

UebelAndre Apr 27, 2021

jheaff1 Apr 28, 2021

jheaff1 Apr 28, 2021

UebelAndre commented Jun 17, 2021

jheaff1 commented Jun 17, 2021

jheaff1 commented Jun 17, 2021

UebelAndre commented Jun 17, 2021

UebelAndre commented Jun 17, 2021

jheaff1 commented Jun 17, 2021

jheaff1 commented Jun 17, 2021 •

edited

Loading

UebelAndre commented Jun 18, 2021

jheaff1 commented Jun 18, 2021

jheaff1 commented Jun 18, 2021 •

edited

Loading

jheaff1 commented Jun 22, 2021

UebelAndre commented Jun 22, 2021

jheaff1 commented Jun 22, 2021

jsharpe commented Jun 22, 2021

jheaff1 commented Jun 22, 2021

UebelAndre left a comment

jsharpe Jun 22, 2021

jsharpe Jun 22, 2021

jsharpe left a comment

jheaff1 commented Jun 22, 2021

	test_suite(
	name = "linux_tests",
	tags = ["manual"],
	tests = [
	"//bison:bison_build_test",
	"//cares:test_c_ares",
	"//curl:curl_build_test",
	"//gn:gn_launch_test",
	"//libgit2:libgit2_build_test",
	"//libpng:test_libpng",
	"//libssh2:libssh2_build_test",
	"//openssl:openssl_build_test",
	"//pcre:pcre_build_test",
	],
	)

	test_suite(
	name = "linux_rbe_tests",
	tags = ["manual"],
	tests = [
	# Missing a new enough m4 to build
	#"//bison:bison_build_test",
	"//cares:test_c_ares",
	"//curl:curl_build_test",
	# Attempts to access git sha during configure of build so fails
	#"//gn:gn_launch_test",
	"//libgit2:libgit2_build_test",
	"//libpng:test_libpng",
	"//libssh2:libssh2_build_test",
	"//openssl:openssl_build_test",
	"//pcre:pcre_build_test",
	],
	)

	test_suite(
	name = "macos_tests",
	tags = ["manual"],
	tests = [
	"//cares:test_c_ares",
	"//curl:curl_build_test",
	"//gn:gn_launch_test",
	"//iconv:iconv_build_test",
	"//libgit2:libgit2_build_test",
	"//libpng:test_libpng",
	"//libssh2:libssh2_build_test",
	"//openssl:openssl_build_test",
	"//pcre:pcre_build_test",
	],
	)

	test_suite(
	name = "windows_tests",
	tags = ["manual"],
	tests = [
	# TODO: Add windows tests
	],
	)

Provide out_data_dirs attribute (#419) #622

Provide out_data_dirs attribute (#419) #622

Conversation

jheaff1 commented Apr 27, 2021

UebelAndre left a comment

Choose a reason for hiding this comment

jheaff1 commented Apr 27, 2021 • edited Loading

jsharpe Apr 27, 2021

Choose a reason for hiding this comment

UebelAndre Apr 27, 2021

Choose a reason for hiding this comment

jheaff1 Apr 28, 2021

Choose a reason for hiding this comment

jheaff1 Apr 28, 2021

Choose a reason for hiding this comment

UebelAndre commented Jun 17, 2021

jheaff1 commented Jun 17, 2021

jheaff1 commented Jun 17, 2021

UebelAndre commented Jun 17, 2021

UebelAndre commented Jun 17, 2021

jheaff1 commented Jun 17, 2021

jheaff1 commented Jun 17, 2021 • edited Loading

UebelAndre commented Jun 18, 2021

jheaff1 commented Jun 18, 2021

jheaff1 commented Jun 18, 2021 • edited Loading

jheaff1 commented Jun 22, 2021

UebelAndre commented Jun 22, 2021

jheaff1 commented Jun 22, 2021

jsharpe commented Jun 22, 2021

jheaff1 commented Jun 22, 2021

UebelAndre left a comment

Choose a reason for hiding this comment

jsharpe Jun 22, 2021

Choose a reason for hiding this comment

jsharpe Jun 22, 2021

Choose a reason for hiding this comment

jsharpe left a comment

Choose a reason for hiding this comment

jheaff1 commented Jun 22, 2021

Provide `out_data_dirs` attribute (#419) #622

Provide `out_data_dirs` attribute (#419) #622

jheaff1 commented Apr 27, 2021 •

edited

Loading

jheaff1 commented Jun 17, 2021 •

edited

Loading

jheaff1 commented Jun 18, 2021 •

edited

Loading