Allow lllc to be installed via Make by Matthalp-zz · Pull Request #3398 · argotorg/solidity

Matthalp-zz · 2018-01-16T15:16:41Z

chriseth · 2018-01-16T15:17:29Z

lllc/CMakeLists.txt

+	# This is not supported on macOS, see
+	# https://developer.apple.com/library/content/qa/qa1118/_index.html.
+	set_target_properties(
+		solc PROPERTIES


This should probably read lllc

chriseth · 2018-01-16T15:18:06Z

In general, I welcome this change. We only have to check whether it adds another executable to the debian / ubuntu builds. lllc should be its own package and not a binary that is part of the solc package.

Matthalp-zz · 2018-01-16T16:16:48Z

Sounds good. Let me know if there is anything I need to do to help.

chriseth · 2018-01-16T17:01:15Z

I'm pretty sure it will just install lllc as well. If you know debian/ubuntu packaging, you can take a look at scripts/release_ppa.sh.

axic · 2018-01-16T18:08:14Z

lllc/CMakeLists.txt

+	# This is not supported on macOS, see
+	# https://developer.apple.com/library/content/qa/qa1118/_index.html.
+	set_target_properties(
+		solc PROPERTIES


Are you sure this shouldn't read lllc?

Matthalp-zz · 2018-01-16T18:16:59Z

@axic good catch -- addressed.

Matthalp-zz · 2018-01-16T18:18:38Z

I don't know much in regard to packaging, but if you feel like more has to be done here in that regard I can take a look. Just let me know.

chfast · 2018-01-16T20:32:27Z

lllc/CMakeLists.txt

 target_link_libraries(lllc PRIVATE lll)

+include(GNUInstallDirs)
+install(TARGETS lllc DESTINATION "${CMAKE_INSTALL_BINDIR}")


Just remove "". They actually can corrupt paths.

@chfast I followed the convention in solc/CMakeLists.txt:

install(TARGETS solc DESTINATION "${CMAKE_INSTALL_BINDIR}")

Yes, I was doing the same, but it turned out it does not anything. And sometimes is harmful.

Matthalp-zz · 2018-01-16T22:42:09Z

@chfast I have removed the quotes in lllc/CMakeLists.txt, but did not address the quotes that remain in the corresponding part of the solc/CMakeLists.txt since that is out of the scope of the changes being made in this PR.

chriseth

Before this can be merged, 'scripts/release_ppa.sh` has to be fixed so that lllc is not installed as part of the solc ubuntu package.

chriseth · 2018-01-17T10:03:17Z

As a compromise, we could probably change this so that make install does not install lllc, but something like make install lllc installs it. Not sure if that is possible.

chfast · 2018-01-17T13:03:25Z

There are 2 solutions for this in CMake:

Don't build lllc by default.

make (does not build lllc)
make install (would install solc without lllc because not built)
make lllc (will build lllc)
make install (would install solc with lllc because built)

I think this is rather confusing, because make install behavior changes depending on what you have currently built. If you want build lllc but not install it you have to remember to remove it before make install.

Use `install(COMPONENT)`

Specify different "components" for solc and lllc, e.g. "solidity" and "extra-tools".
To install single component, you have to do cmake -DCOMPONENT=comp_one -P {your_build_dir}/cmake_install.cmake. Not very beautiful, but works.

We can also have a combination of both.

Matthalp-zz · 2018-01-17T13:44:47Z

Can I propose a third option where there is a CMAKE option to specify whether lllc should be included in make install? I went ahead and updated the files to show the changes that are required to support this.

cmake -DLLLC_INSTALL=ON ..

chfast · 2018-01-17T18:41:48Z

cmake -DLLLC_INSTALL=ON ..

Yes, this is good idea. However I'd call it INSTALL_LLLC.

Matthalp-zz · 2018-01-17T18:45:45Z

I'm fine with whatever name you and @chriseth agree on.

chriseth · 2018-01-19T15:45:35Z

@matthalp a cmake flag is a good idea! I think we can actually default it to install lllc and then add the setting in the scripts/ppa_relaese.sh:


override_dh_auto_configure:
        dh_auto_configure -- -DINSTALL_LLLC=Off

But someone would have to test that :)

axic · 2018-01-24T02:50:32Z

@matthalp can you make the changes to the release scripts to turn off installation (as mentioned by @chriseth)?

Matthalp-zz · 2018-01-25T17:51:52Z

The change is easy, but I still have to sit down and test it locally. I'll try to do that by the end of the weekend.

xemuliam · 2018-02-08T11:38:04Z

Interesting thing: most of Solidity manuals say "if you have installed solc you've already had lllc also".
However lllc is absent in Ethereum PPA and it can't be installed via source code compilation without manual intervention.
BTW both of them solc and lllc take a part of compilation and making. Onnly one difference is installation: it works only for solc.
Why it is important? It is only yet another one compiler for contracts. What's wrong with addition of it to standard make install?

Matthalp-zz · 2018-02-10T04:18:29Z

@axic @chriseth Where do you stand based on the comment from @xemuliam? Should I exclude lllc from being installed as part of the Debian package or not?

chriseth · 2018-02-10T12:51:17Z

Yes, lllc should have its own debian package.

axic · 2018-02-12T14:00:36Z

CMakeLists.txt


 option(SOLC_LINK_STATIC "Link solc executable statically on supported platforms" OFF)
+option(LLLC_LINK_STATIC "Link lllc executable statically on supported platforms" OFF)
+option(INSTALL_LLLC "Include lllc executable in installation" ON)


How about making this off by default?

After having read up the earlier comments, the benefits are:

it definitely won't alter the ppa process

it will not give the impression that lllc is a fully maintained, supported and integral part of solc (as it is just something allowed to exists here)

I'm fine either way. Most people don't build from source anyway.

Let's just get this merged today so we can create a release tomorrow.

axic · 2018-02-13T10:21:52Z

@chriseth approve then?

Matthalp-zz · 2018-02-13T11:17:47Z

scripts/release_ppa.sh

 override_dh_shlibdeps:
 	dh_shlibdeps --dpkg-shlibdeps-params=--ignore-missing-info
+
+override_dh_auto_configure:


@axic If INSTALL_LLLC is off by default then this can be removed.

Yes, but kept it there for safety reasons. We may switch it on or who knows :)

LianaHus added in progress and removed in progress labels Jan 16, 2018

chriseth reviewed Jan 16, 2018

View reviewed changes

Matthalp-zz force-pushed the install-lllc branch from 160c860 to 05a625e Compare January 16, 2018 16:07

axic reviewed Jan 16, 2018

View reviewed changes

axic requested a review from chfast January 16, 2018 18:08

Matthalp-zz force-pushed the install-lllc branch from 05a625e to 9dbc141 Compare January 16, 2018 18:16

chfast suggested changes Jan 16, 2018

View reviewed changes

Matthalp-zz force-pushed the install-lllc branch from 9dbc141 to afcb0be Compare January 16, 2018 22:40

chfast approved these changes Jan 16, 2018

View reviewed changes

chriseth suggested changes Jan 17, 2018

View reviewed changes

Matthalp-zz force-pushed the install-lllc branch from afcb0be to c8a479d Compare January 17, 2018 13:42

Matthalp-zz force-pushed the install-lllc branch from c8a479d to 4d58011 Compare January 19, 2018 16:27

Matthalp-zz force-pushed the install-lllc branch from 4d58011 to 745f633 Compare January 29, 2018 13:59

axic mentioned this pull request Feb 7, 2018

Fix: Unable to install LLLC beside SOLC #3468

Closed

axic added the nextrelease label Feb 9, 2018

axic reviewed Feb 12, 2018

View reviewed changes

Matthew Halpern and others added 2 commits February 13, 2018 10:20

Allow lllc to be installed

da36c82

Turn off lllc installation by default

52e55b7

axic force-pushed the install-lllc branch from 745f633 to 52e55b7 Compare February 13, 2018 10:21

Matthalp-zz commented Feb 13, 2018

View reviewed changes

chriseth approved these changes Feb 13, 2018

View reviewed changes

chriseth merged commit 1d21f30 into argotorg:develop Feb 13, 2018

LianaHus removed needs review labels Feb 13, 2018

Conversation

Matthalp-zz commented Jan 16, 2018 • edited by axic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Jan 16, 2018

Uh oh!

Matthalp-zz commented Jan 16, 2018

Uh oh!

chriseth commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matthalp-zz commented Jan 16, 2018

Uh oh!

Matthalp-zz commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matthalp-zz commented Jan 16, 2018

Uh oh!

chriseth left a comment

Choose a reason for hiding this comment

Uh oh!

chriseth commented Jan 17, 2018

Uh oh!

chfast commented Jan 17, 2018

Don't build lllc by default.

Use install(COMPONENT)

Uh oh!

Matthalp-zz commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chfast commented Jan 17, 2018

Uh oh!

Matthalp-zz commented Jan 17, 2018

Uh oh!

chriseth commented Jan 19, 2018

Uh oh!

axic commented Jan 24, 2018

Uh oh!

Matthalp-zz commented Jan 25, 2018

Uh oh!

xemuliam commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matthalp-zz commented Feb 10, 2018

Uh oh!

chriseth commented Feb 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Feb 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Matthalp-zz commented Jan 16, 2018 •

edited by axic

Loading

Use `install(COMPONENT)`

Matthalp-zz commented Jan 17, 2018 •

edited

Loading

xemuliam commented Feb 8, 2018 •

edited

Loading