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

hls/xls: disable conda-forge #219

Merged
merged 1 commit into from
Aug 19, 2022
Merged

Conversation

proppy
Copy link
Contributor

@proppy proppy commented Jun 22, 2022

  • remove conda-forge requirements
  • install bazel using bazelisk
  • remove obsolete patches
  • strip python host dependencies and patches (not used by binary rules)

/cc @cdleary @mithro

@proppy proppy requested a review from ajelinski June 22, 2022 07:03
@proppy
Copy link
Contributor Author

proppy commented Jun 22, 2022

was getting the following warnings:

WARNING (xls,bin/ir_converter_main): $RPATH/libstdc++.so.6 not found in packages, sysroot(s) nor the missing_dso_whitelist.
.. is this binary repackaging?

which turns into the following errors after 0a208ec

  ERROR (xls,bin/codegen_main): $PATH/lib/libstdc++.so.6 found in build prefix; should never happen

Copy link
Contributor

@ajelinski ajelinski left a comment

Choose a reason for hiding this comment

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

I'm not sure... Are the binaries independent of numpy, scipy and all other packages removed from the requirements? The bazel change seems OK if the Conda package is problematic, I guess it isn't needed to run the package?

I'd also like to check log if the binaries aren't linked against the VM's libraries instead of the Conda-packaged ones but the package building is currently skipped by the CI: https://github.com/hdl/conda-eda/runs/6999838565?check_suite_focus=true#step:3:11

Please remove this line to restore the package building in CI: https://github.com/hdl/conda-eda/blob/master/.github/workflows/Build.yml#L763

@ajelinski
Copy link
Contributor

was getting the following warnings:

WARNING (xls,bin/ir_converter_main): $RPATH/libstdc++.so.6 not found in packages, sysroot(s) nor the missing_dso_whitelist.
.. is this binary repackaging?

which turns into the following errors after 0a208ec

  ERROR (xls,bin/codegen_main): $PATH/lib/libstdc++.so.6 found in build prefix; should never happen

Never seen such an error, it's quite weird. Does bazel install its own libstdc++ library to to link built executables against?

@proppy
Copy link
Contributor Author

proppy commented Aug 16, 2022

@ajelinski the build looks clean now (no ERROR just missing_dso_whitelist warnings), can you review?

@ajelinski
Copy link
Contributor

@proppy This is now built completely outside Conda which isn't really a preferred way of building Conda packages.
Is it the only way to fix build problems?

Currently libc libraries are required: https://github.com/hdl/conda-eda/runs/7854507476?check_suite_focus=true#step:3:15915
Does bazel use libc simply from the OS or does it provide its own version to make sure you can run it with older libc than the one used during building?

@proppy
Copy link
Contributor Author

proppy commented Aug 18, 2022

This is now built completely outside Conda which isn't really a preferred way of building Conda packages.

Wasn't this already the case were we were using the bazel conda-forge package w/ the builtin llvm toolchain? (did switching bazelisk in order to get rid of the conda-forge dep had other implication?)

@proppy
Copy link
Contributor Author

proppy commented Aug 18, 2022

I wonder if the libc WARNING are due to the patchelf failure just above:

2022-08-16T10:45:52.3087169Z patchelf: open: Permission denied
2022-08-16T10:45:52.3978373Z patchelf: open: Permission denied
2022-08-16T10:45:52.8803411Z patchelf: open: Permission denied
2022-08-16T10:45:53.0252717Z patchelf: open: Permission denied
2022-08-16T10:45:53.0749604Z patchelf: open: Permission denied

@proppy
Copy link
Contributor Author

proppy commented Aug 18, 2022

For reference using the conda-forge version of bazel in #233 resulted in a crash
https://github.com/hdl/conda-eda/runs/7833018668?check_suite_focus=true

FATAL: bazel crashed due to an internal error. Printing stack trace:
  java.lang.ExceptionInInitializerError
  	at com.google.devtools.build.lib.actions.ParameterFile.writeContent(ParameterFile.java:118)
  	at com.google.devtools.build.lib.actions.ParameterFile.writeParameterFile(ParameterFile.java:111)
  	at com.google.devtools.build.lib.analysis.actions.ParameterFileWriteAction$ParamFileWriter.writeOutputFile(ParameterFileWriteAction.java:170)
  	at com.google.devtools.build.lib.exec.FileWriteStrategy.beginWriteOutputToFile(FileWriteStrategy.java:58)
  	at com.google.devtools.build.lib.analysis.actions.FileWriteActionContext.beginWriteOutputToFile(FileWriteActionContext.java:49)
  	at com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction.beginExecution(AbstractFileWriteAction.java:66)
  	at com.google.devtools.build.lib.actions.Action.execute(Action.java:133)
  	at com.google.devtools.build.lib.skyframe.SkyframeActionExecutor$5.execute(SkyframeActionExecutor.java:907)
  	at com.google.devtools.build.lib.skyframe.SkyframeActionExecutor$ActionRunner.continueAction(SkyframeActionExecutor.java:1076)
  	at com.google.devtools.build.lib.skyframe.SkyframeActionExecutor$ActionRunner.run(SkyframeActionExecutor.java:1031)
  	at com.google.devtools.build.lib.skyframe.ActionExecutionState.runStateMachine(ActionExecutionState.java:152)
  	at com.google.devtools.build.lib.skyframe.ActionExecutionState.getResultOrDependOnFuture(ActionExecutionState.java:91)
  	at com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.executeAction(SkyframeActionExecutor.java:492)
  	at com.google.devtools.build.lib.skyframe.ActionExecutionFunction.checkCacheAndExecuteIfNeeded(ActionExecutionFunction.java:856)
  	at com.google.devtools.build.lib.skyframe.ActionExecutionFunction.computeInternal(ActionExecutionFunction.java:349)
  	at com.google.devtools.build.lib.skyframe.ActionExecutionFunction.compute(ActionExecutionFunction.java:169)
  	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:590)
  	at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:382)
  	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
  	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
  	at java.base/java.lang.Thread.run(Thread.java:833)
  Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make java.lang.String(byte[],byte) accessible: module java.base does not "opens java.lang" to unnamed module @18494e8a
  	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
  	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
  	at java.base/java.lang.reflect.Constructor.checkCanSetAccessible(Constructor.java:188)
  	at java.base/java.lang.reflect.Constructor.setAccessible(Constructor.java:181)
  	at com.google.devtools.build.lib.unsafe.StringUnsafe.<init>(StringUnsafe.java:75)
  	at com.google.devtools.build.lib.unsafe.StringUnsafe.initInstance(StringUnsafe.java:56)
  	at com.google.devtools.build.lib.unsafe.StringUnsafe.<clinit>(StringUnsafe.java:37)
  	... 21 more

@proppy
Copy link
Contributor Author

proppy commented Aug 18, 2022

Seems like this got in fixed fixed in 5.2.0 bazelbuild/bazel#14548, but somehow still trigger for us with the version packaged in conda: bazel-5.2.0-hfe0e5f3_0.tar.bz2

@ajelinski
Copy link
Contributor

ajelinski commented Aug 18, 2022

This is now built completely outside Conda which isn't really a preferred way of building Conda packages.

Wasn't this already the case were we were using the bazel conda-forge package w/ the builtin llvm toolchain? (did switching bazelisk in order to get rid of the conda-forge dep had other implication?)

Oh yeah, there indeed was a problem with using a non-Conda compiler before: #199... unfortunately it will still be a thing, especially when the Ubuntu 18.04 gets removed later this year (actions/runner-images#6002) and such an xls package built on Ubuntu 20.04 won't work on Debian Buster...

I don't oppose the changes strongly, I'm only worried about whether the new package won't turn out to be broken for some users but in this case it seems the changes don't influence this aspect.
Have you tried to install the package and use the executables built -- especially in a clean Conda environment?

BTW. It seems the Java problem can be fixed by using openjdk v11, e.g.: https://github.com/exasol/script-languages/pull/321/files
Still, if the previous requirements are only necessary to build the package but not for running or are embedded in the package, I'm fine with removing them.

@ajelinski
Copy link
Contributor

I wonder if the libc WARNING are due to the patchelf failure just above:

2022-08-16T10:45:52.3087169Z patchelf: open: Permission denied
2022-08-16T10:45:52.3978373Z patchelf: open: Permission denied
2022-08-16T10:45:52.8803411Z patchelf: open: Permission denied
2022-08-16T10:45:53.0252717Z patchelf: open: Permission denied
2022-08-16T10:45:53.0749604Z patchelf: open: Permission denied

No, the libc warnings are caused by the fact that executables are linked to the glibc libraries and a Conda-based compiler (which provides an old glibc v2.17 to prevent requiring any newer glibc) wasn't used during building.

@proppy
Copy link
Contributor Author

proppy commented Aug 18, 2022

Oh yeah, there indeed was a problem with using a non-Conda compiler before: #199... unfortunately it will still be a thing, especially when the Ubuntu 18.04 gets removed later this year (actions/runner-images#6002) and such an xls package built on Ubuntu 20.04 won't work on Debian Buster...

Yep, I think this PR doesn't improve of that (but doesn't make it worst either), we're mainly just remove the bazel/conda-forge package in favor of bazelisk to get rid of the conda-forge dependency, but it doesn't fundamentally change how the package is built (it was already not using the conda compiler as pointed in #199).

Have you tried to install the package and use the executables built -- especially in a clean Conda environment?

Yep, tried it with https://github.com/proppy/rad-lab (which is 20.04 based).

BTW. It seems the Java problem can be fixed by using openjdk v11, e.g.: https://github.com/exasol/script-languages/pull/321/files

Would you rather have a separate PR that simply fix bazel + conda-forge to renable the CI, and rebase this one on top to keep it just about conda-forge?

Still, if the previous requirements are only necessary to build the package but not for running or are embedded in the package, I'm fine with removing them.

I don't think previous requirements were actually being used by the build.

@proppy
Copy link
Contributor Author

proppy commented Aug 18, 2022

No, the libc warnings are caused by the fact that executables are linked to the glibc libraries and a Conda-based compiler (which provides an old glibc v2.17 to prevent requiring any newer glibc) wasn't used during building.

My shallow understanding of conda-build was that it sometime used patchelf to rewrite the path/version of some of the dependencies, but I guess it's not attempting to do that with implicit system dependencies like the glibc?

@ajelinski
Copy link
Contributor

Please rebase and I'll merge the changes since they fix building and the package has no more problems than before.

BTW. It seems the Java problem can be fixed by using openjdk v11, e.g.: https://github.com/exasol/script-languages/pull/321/files

Would you rather have a separate PR that simply fix bazel + conda-forge to renable the CI, and rebase this one on top to keep it just about conda-forge?

No, in this particular case I think it's OK as it is. I was just wondering if you've seen it.

No, the libc warnings are caused by the fact that executables are linked to the glibc libraries and a Conda-based compiler (which provides an old glibc v2.17 to prevent requiring any newer glibc) wasn't used during building.

My shallow understanding of conda-build was that it sometime used patchelf to rewrite the path/version of some of the dependencies, but I guess it's not attempting to do that with implicit system dependencies like the glibc?

TBH I didn't know what patchelf does when packaging but I've just checked it modifies RPATH to an $ORIGIN-based path (e.g. $ORIGIN/../lib). It doesn't apply to the glibc, however, since Conda doesn't provide it apart from the sysroot package used only by compilers.

- remove conda-forge requirements
- install bazel using bazelisk
- remove obsolete patches
- strip python host dependencies and patches (not used by binary rules)

hls/xls: add compiler deps

github/workflows/Build: re-enable xls

hls/xls: remove requirements

hls/xls: add proto_to_dslx_main

hls/xls: re-enable llvm toolchain

hls/xls: build proto_to_dslx_main

hls/xls: remove bazel alias
@proppy proppy force-pushed the xls-no-conda-forge branch from 8712466 to c8d203d Compare August 19, 2022 14:36
@proppy
Copy link
Contributor Author

proppy commented Aug 19, 2022

Please rebase

Done.

@proppy proppy requested a review from ajelinski August 19, 2022 14:37
@ajelinski ajelinski merged commit 813887b into hdl:master Aug 19, 2022
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