Issue/347/llvmpath #413

vincentmr · 2023-02-03T19:27:48Z

Context:
conda-forge builders cannot use brew to get the root of the llvm installation which prevents creating up-to-date PL-Lightning Conda-Forge packages.

Description of the Change:
Allow getting the llvm installation root from the environment variable LLVM_ROOT_DIR.

Benefits:
Maintain PL-Lightning Conda-Forge packages.

Possible Drawbacks:
None.

Related GitHub Issues:
#347

- .github/workflows/dev_version_script.py - .github/workflows/vb_script.py - bin/utils.py - doc/conf.py - setup.py

…on Darwin systems.

github-actions · 2023-02-03T19:28:07Z

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

A one-to-two sentence description of the change. You may include a small working example for new features.
A link back to this PR.
Your name (or GitHub username) in the contributors section.

codecov · 2023-02-03T19:45:32Z

Codecov Report

Merging #413 (f5d3196) into master (b037f5e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #413   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files          49       49           
  Lines        4517     4517           
=======================================
  Hits         4509     4509           
  Misses          8        8

Impacted Files	Coverage Δ
pennylane_lightning/_version.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

mlxd

Thanks @vincentmr
I'll aim to test this against a non brew LLVM installation with the env var and confirm all is well.

Also, did the linter make the quote/whitespace/newline changes?

.github/workflows/dev_version_script.py

AmintorDusko

Thank you for tackling this issue.

doc/conf.py

Co-authored-by: Amintor Dusko <87949283+AmintorDusko@users.noreply.github.com>

Conflicts: doc/conf.py pennylane_lightning/_version.py setup.py

mlxd

Thanks @vincentmr
Happy to approve

vincentmr · 2023-02-06T22:22:54Z

@mlxd I had in mind that the compiler would necessarily by somepath/bin/clang++, but I wonder whether it will be the case in the conda-forge CI. If not, maybe we should just pick a variable like CMAKE_CXX_COMPILER with the full path. What do you think?

mlxd · 2023-02-06T22:36:19Z

@mlxd I had in mind that the compiler would necessarily by somepath/bin/clang++, but I wonder whether it will be the case in the conda-forge CI. If not, maybe we should just pick a variable like CMAKE_CXX_COMPILER with the full path. What do you think?

I think it is fine to assume the path will be something/bin/clang++. If this is not the case, we can make a follow-up PR with input from conda-forge build. I'd rather avoid setting the CMAKE_CXX_COMPILER and CMAKE_LINKER directly from CLI unless we need to, but happy to move this if it makes sense to.

.github/CHANGELOG.md

Co-authored-by: Amintor Dusko <87949283+AmintorDusko@users.noreply.github.com>

vincentmr · 2023-02-07T13:43:19Z

I think it is fine to assume the path will be something/bin/clang++. If this is not the case, we can make a follow-up PR with input from conda-forge build. I'd rather avoid setting the CMAKE_CXX_COMPILER and CMAKE_LINKER directly from CLI unless we need to, but happy to move this if it makes sense to.

I agree, but I was wondering since the conda-forge build outputs paths such as

export PREFIX=/Users/runner/miniforge3/conda-bld/pennylane-lightning_1675714784916/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold
export BUILD_PREFIX=/Users/runner/miniforge3/conda-bld/pennylane-lightning_1675714784916/_build_env
+CC=x86_64-apple-darwin13.4.0-clang
+CC_FOR_BUILD=$BUILD_PREFIX/bin/x86_64-apple-darwin13.4.0-clang
+CFLAGS=-march=core2 -mtune=haswell -mssse3 -ftree-vectorize -fPIC -fPIE -fstack-protector-strong -O2 -pipe -isystem $PREFIX/include -fdebug-prefix-map=$SRC_DIR=/usr/local/src/conda/pennylane-lightning-0.28.0 -fdebug-prefix-map=$PREFIX=/usr/local/src/conda-prefix
+CLANG=x86_64-apple-darwin13.4.0-clang
+LD=x86_64-apple-darwin13.4.0-ld
+LDFLAGS=-Wl,-pie -Wl,-headerpad_max_install_names -Wl,-dead_strip_dylibs -Wl,-rpath,$PREFIX/lib -L$PREFIX/lib
+LDFLAGS_LD=-pie -headerpad_max_install_names -dead_strip_dylibs -rpath $PREFIX/lib -L$PREFIX/lib
INFO: activate_clangxx_osx-64.sh made the following environmental changes:
+CLANGXX=x86_64-apple-darwin13.4.0-clang++
+CXX=x86_64-apple-darwin13.4.0-clang++
+CXXFLAGS=-march=core2 -mtune=haswell -mssse3 -ftree-vectorize -fPIC -fPIE -fstack-protector-strong -O2 -pipe -stdlib=libc++ -fvisibility-inlines-hidden -fmessage-length=0 -isystem $PREFIX/include -fdebug-prefix-map=$SRC_DIR=/usr/local/src/conda/pennylane-lightning-0.28.0 -fdebug-prefix-map=$PREFIX=/usr/local/src/conda-prefix
+CXX_FOR_BUILD=$BUILD_PREFIX/bin/x86_64-apple-darwin13.4.0-clang++

and in the Anaconda doc compilers include platform-dependent prefixes/suffixes. After spawning a container, I'm not quite able to reproduce all the build steps and I'm wondering whether the proper symlinks are created down the line.

mlxd · 2023-02-07T14:04:13Z

I think it is fine to assume the path will be something/bin/clang++. If this is not the case, we can make a follow-up PR with input from conda-forge build. I'd rather avoid setting the CMAKE_CXX_COMPILER and CMAKE_LINKER directly from CLI unless we need to, but happy to move this if it makes sense to.

I agree, but I was wondering since the conda-forge build outputs paths such as

export PREFIX=/Users/runner/miniforge3/conda-bld/pennylane-lightning_1675714784916/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold
export BUILD_PREFIX=/Users/runner/miniforge3/conda-bld/pennylane-lightning_1675714784916/_build_env
+CC=x86_64-apple-darwin13.4.0-clang
+CC_FOR_BUILD=$BUILD_PREFIX/bin/x86_64-apple-darwin13.4.0-clang
+CFLAGS=-march=core2 -mtune=haswell -mssse3 -ftree-vectorize -fPIC -fPIE -fstack-protector-strong -O2 -pipe -isystem $PREFIX/include -fdebug-prefix-map=$SRC_DIR=/usr/local/src/conda/pennylane-lightning-0.28.0 -fdebug-prefix-map=$PREFIX=/usr/local/src/conda-prefix
+CLANG=x86_64-apple-darwin13.4.0-clang
+LD=x86_64-apple-darwin13.4.0-ld
+LDFLAGS=-Wl,-pie -Wl,-headerpad_max_install_names -Wl,-dead_strip_dylibs -Wl,-rpath,$PREFIX/lib -L$PREFIX/lib
+LDFLAGS_LD=-pie -headerpad_max_install_names -dead_strip_dylibs -rpath $PREFIX/lib -L$PREFIX/lib
INFO: activate_clangxx_osx-64.sh made the following environmental changes:
+CLANGXX=x86_64-apple-darwin13.4.0-clang++
+CXX=x86_64-apple-darwin13.4.0-clang++
+CXXFLAGS=-march=core2 -mtune=haswell -mssse3 -ftree-vectorize -fPIC -fPIE -fstack-protector-strong -O2 -pipe -stdlib=libc++ -fvisibility-inlines-hidden -fmessage-length=0 -isystem $PREFIX/include -fdebug-prefix-map=$SRC_DIR=/usr/local/src/conda/pennylane-lightning-0.28.0 -fdebug-prefix-map=$PREFIX=/usr/local/src/conda-prefix
+CXX_FOR_BUILD=$BUILD_PREFIX/bin/x86_64-apple-darwin13.4.0-clang++

and in the Anaconda doc compilers include platform-dependent prefixes/suffixes. After spawning a container, I'm not quite able to reproduce all the build steps and I'm wondering whether the proper symlinks are created down the line.

Ah, I understand now. In that case, since we cannot control the compiler triples/paths, it probably is the path of least resistance to just pass in the direct path to CMAKE_CXX_COMPILER. Feel free to ignore my previous comment and go for it. Though, I wonder if we should opt for this to be a fully general solution in the setup.py for all target platforms, rather than just for MacOS. What do you think?

vincentmr · 2023-02-07T14:16:29Z

In that case, since we cannot control the compiler triples/paths, it probably is the path of least resistance to just pass in the direct path to CMAKE_CXX_COMPILER. Feel free to ignore my previous comment and go for it. Though, I wonder if we should opt for this to be a fully general solution in the setup.py for all target platforms, rather than just for MacOS. What do you think?

Since we are modifying setup.py mainly so that the OSX conda-forge build passes, I would wait until this is fixed and then clean up the solution that ended up working.

mlxd · 2023-02-07T14:25:46Z

In that case, since we cannot control the compiler triples/paths, it probably is the path of least resistance to just pass in the direct path to CMAKE_CXX_COMPILER. Feel free to ignore my previous comment and go for it. Though, I wonder if we should opt for this to be a fully general solution in the setup.py for all target platforms, rather than just for MacOS. What do you think?

Since we are modifying setup.py mainly so that the OSX conda-forge build passes, I would wait until this is fixed and then clean up the solution that ended up working.

Sounds good. I'll revoke approval and review again once the new path changes are added.

Awaiting updates for compiler paths

vincentmr · 2023-02-14T19:19:43Z

I have not had specific feedback from Bastian on the changes here, but the changes allow building PL and PLL on conda-forge, which was the main goal (see conda-forge/pennylane-lightning-feedstock#16). So I think the PR could move forward and be merged when ready.

AmintorDusko · 2023-02-15T16:58:24Z

We need #416 merged before reviewing this one.

mlxd

Thanks for the work here @vincentmr
If this solves the issues #347 happy to approve

AmintorDusko

Thank you, @vincentmr!

AmintorDusko · 2023-02-22T15:54:07Z

Hey @vincentmr, don't forget to update the branch reference in conda-forge after merging this PR.

vincentmr · 2023-02-22T16:13:27Z

Hey @vincentmr, don't forget to update the branch reference in conda-forge after merging this PR.

Good point. I'll make a few more tests, then merge. Thanks for your reviews.

vincentmr and others added 4 commits February 3, 2023 14:02

Run black -l 100 on:

01e7576

- .github/workflows/dev_version_script.py - .github/workflows/vb_script.py - bin/utils.py - doc/conf.py - setup.py

Add LLVM_ROOT_DIR variable that allows specifying a custom llvm path …

c081924

…on Darwin systems.

Fix a couple typos in README file.

7f09d3d

Auto update version

728efa0

AmintorDusko linked an issue Feb 3, 2023 that may be closed by this pull request

Don't use brew for llvm path #347

Closed

mlxd reviewed Feb 3, 2023

View reviewed changes

.github/workflows/dev_version_script.py Outdated Show resolved Hide resolved

AmintorDusko reviewed Feb 3, 2023

View reviewed changes

doc/conf.py Outdated Show resolved Hide resolved

vincentmr and others added 7 commits February 3, 2023 17:15

Update doc/conf.py

5db93c5

Co-authored-by: Amintor Dusko <87949283+AmintorDusko@users.noreply.github.com>

Auto update version

f117379

Update changelog

fafb5fb

Merge remote-tracking branch 'origin/master' into issue/347/llvmpath

648fb94

Trigger CI.

013b7c8

Merge remote-tracking branch 'origin/master' into issue/347/llvmpath

d314755

Conflicts: doc/conf.py pennylane_lightning/_version.py setup.py

Auto update version

757fb91

mlxd previously approved these changes Feb 6, 2023

View reviewed changes

AmintorDusko reviewed Feb 7, 2023

View reviewed changes

.github/CHANGELOG.md Outdated Show resolved Hide resolved

Update .github/CHANGELOG.md

f63c5fe

Co-authored-by: Amintor Dusko <87949283+AmintorDusko@users.noreply.github.com>

vincentmr and others added 4 commits February 7, 2023 10:16

If brew not found, find path using which clang++.

69900a6

Auto update version

f7f3836

Merge branch 'master' into issue/347/llvmpath

39d1287

Remove tmp var brewpath and merge few elseif clauses.

b3a6791

vincentmr requested review from AmintorDusko and mlxd February 15, 2023 14:43

mlxd and others added 2 commits February 21, 2023 17:45

Merge branch 'master' into issue/347/llvmpath

f28ad20

Auto update version

f5d3196

mlxd approved these changes Feb 22, 2023

View reviewed changes

AmintorDusko approved these changes Feb 22, 2023

View reviewed changes

vincentmr merged commit 6b58585 into master Feb 22, 2023

vincentmr deleted the issue/347/llvmpath branch February 22, 2023 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/347/llvmpath #413

Issue/347/llvmpath #413

vincentmr commented Feb 3, 2023

github-actions bot commented Feb 3, 2023

codecov bot commented Feb 3, 2023 •

edited

Loading

mlxd left a comment

AmintorDusko left a comment

mlxd left a comment

vincentmr commented Feb 6, 2023

mlxd commented Feb 6, 2023

vincentmr commented Feb 7, 2023

mlxd commented Feb 7, 2023

vincentmr commented Feb 7, 2023

mlxd commented Feb 7, 2023

vincentmr commented Feb 14, 2023

AmintorDusko commented Feb 15, 2023

mlxd left a comment

AmintorDusko left a comment

AmintorDusko commented Feb 22, 2023

vincentmr commented Feb 22, 2023

Issue/347/llvmpath #413

Issue/347/llvmpath #413

Conversation

vincentmr commented Feb 3, 2023

github-actions bot commented Feb 3, 2023

codecov bot commented Feb 3, 2023 • edited Loading

Codecov Report

mlxd left a comment

Choose a reason for hiding this comment

AmintorDusko left a comment

Choose a reason for hiding this comment

mlxd left a comment

Choose a reason for hiding this comment

vincentmr commented Feb 6, 2023

mlxd commented Feb 6, 2023

vincentmr commented Feb 7, 2023

mlxd commented Feb 7, 2023

vincentmr commented Feb 7, 2023

mlxd commented Feb 7, 2023

vincentmr commented Feb 14, 2023

AmintorDusko commented Feb 15, 2023

mlxd left a comment

Choose a reason for hiding this comment

AmintorDusko left a comment

Choose a reason for hiding this comment

AmintorDusko commented Feb 22, 2023

vincentmr commented Feb 22, 2023

codecov bot commented Feb 3, 2023 •

edited

Loading