/snapcraft

Redo pkg-config handling with library collecting #335

Merged
merged 1 commit into from Mar 30, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@sergiusens @elopio @kyrofa @coveralls @chipaca @didrocks
@sergiusens
Collaborator

sergiusens commented Feb 25, 2016

pkg-config only handled PKG_CONFIG_SYSROOT_DIR as the stage dir
providing many issues when using packages from installdir for a part
and also build-packages from the host.

For a full implementation, elf dependency tracking from the host
to the snap is also implemented here

LP: #1549570

Signed-off-by: Sergio Schvezov sergio.schvezov@ubuntu.com
libraries/16.04
+libustr-1.0.so.1
+libustr-1.0.so.1.0.4
+libyaml-0.so.2
+libyaml-0.so.2.0.4
@elopio

elopio Feb 25, 2016

Member

How did you generate this list?
A README file next to this one explaining why and how would be great.

@sergiusens

sergiusens Feb 25, 2016

Collaborator

Sounds like a good idea, so far I did it manually with find from an OS snap. This will probably need updating from my Qt comments from last night

@kyrofa

kyrofa Feb 25, 2016

Member

Agreed, this could use some documentation.

@sergiusens

sergiusens Feb 29, 2016

Collaborator

@elopio and @kyrofa done https://github.com/ubuntu-core/snapcraft/pull/335/files#diff-2cb561c5e46512465f54a0f84d421c80R66

But is also makes me think if it is worth to make this dynamic.

@kyrofa

kyrofa Feb 29, 2016

Member

How would you go about making it dynamic? Would you have to download the OS snap every time?

snapcraft/common.py
@@ -30,6 +31,9 @@
_plugindir = _DEFAULT_PLUGINDIR
_DEFAULT_SCHEMADIR = '/usr/share/snapcraft/schema'
_schemadir = _DEFAULT_SCHEMADIR
+_DEFAULT_LIBRARIESDIR = 'usr/share/snapcraft/libraries'
@elopio

elopio Feb 25, 2016

Member

_DEFAULT_LIBRARIES_DIR

@elopio

elopio Feb 25, 2016

Member

ah, nevermind, it's following the style of the other constants.

@sergiusens

sergiusens Feb 25, 2016

Collaborator

yeah; I really want to to a massive replace of all the <something>dir to be <something>_dir

snapcraft/common.py
- **kwargs).decode('utf8').strip()
+ return subprocess.check_output([
+ '/bin/sh', f.name] + cmd, **kwargs).decode(
+ sys.getfilesystemencoding()).strip()
@elopio

elopio Feb 25, 2016

Member

This looks good, but it means that if f.name prints a character that's not ascii when the system doesn't have utf8, it will raise an exception. This was the problem we had in a couple of places. So this possible error might need better handling.

@sergiusens

sergiusens Feb 25, 2016

Collaborator

I went under the assumption that stdout and stderr are also just files. In fairness I was trying to get rid of this error:
'utf-8' codec can't encode character '\\udcc3' in position 137:\nsurrogates not allowed\n"

from http://162.213.35.179:8080/job/github-snapcraft-integration-tests-cloud/23/testReport/tests/TestSnapcraftExamples/test_example_java_hello_world_/

@chipaca

chipaca Feb 25, 2016

Member

This changes the signature of run_output from returning a string to returning bytes. This on its own won't raise an exception, but the caller needs to handle it differently from before (because it's different).

snapcraft/libraries.py
+ lib_path = os.path.join(common.get_librariesdir(), release)
+
+ if not os.path.exists(lib_path):
+ logger.warning('No libraries to exclude from this release')
@elopio

elopio Feb 25, 2016

Member

From the text in this message, it's not clear why it is a warning. How can affect the execution that there are no libraries to exclude?

@sergiusens

sergiusens Feb 25, 2016

Collaborator

ah, logger.debug was supposed to be it

@elopio
Member

elopio commented Feb 25, 2016

lgtm, but the elf bits are flying over my head. I'll read more about that tomorrow.
You might want to get chipaca or mvo to take a look.
snapcraft/yaml.py
+ try:
+ check_call(['/usr/bin/pkg-config', '--exists'] + modules, env=env)
+ return True
+ except:
@elopio

elopio Feb 25, 2016

Member

except subprocess.CalledProcessError.
We don't want to ignore any other exceptions.

@sergiusens
Collaborator

sergiusens commented Feb 25, 2016

@elopio wrt ELF I can ldd against ELF (Executable Linking Format) files. I am walking the whole tree in case someone goes with a non standard layout.
snapcraft/libraries.py
+ ldd_out = [l[2] for l in ldd_out if len(l) > 2 and os.path.exists(l[2])]
+
+ # Anything that is part of the project we don't need to check for
+ libs = list(filter(lambda x: not x.startswith(os.getcwd()), ldd_out))
@chipaca

chipaca Feb 25, 2016

Member

two (silly) things that bother me about this line: you're calling os.getcwd in a loop without expecting it to change, and you switched from list comprehensions to filter+lambda.

snapcraft/libraries.py
+
+ # Now lets filter out what would be on the system
+ system_libs = _get_system_libs()
+ libs = list(filter(lambda x: not os.path.basename(x) in system_libs, libs))
@chipaca

chipaca Feb 25, 2016

Member

and here it seems you switched to filter+lambda entirely. Not sure I understand why you're using both idioms in a single function (and why you use filter but flatten it into a list for an intermediate result)

snapcraft/libraries.py
+ logger.warning('No libraries to exclude from this release')
+
+ with open(lib_path) as fn:
+ _libraries = fn.read().split()
@chipaca

chipaca Feb 25, 2016

Member

From what I can tell, you could make _libraries a frozenset and carry on using it as you are, but slightly faster.

snapcraft/pluginhandler.py
+ logger.debug('Skipped link {!r} when parsing {!r}'.format(
+ path, workdir))
+ continue
+ if ms.file(path).startswith('ELF'):
@chipaca

chipaca Feb 25, 2016

Member

you should check it's a dynamic executable, also

@sergiusens
Collaborator

sergiusens commented Feb 25, 2016

thanks @chipaca
@sergiusens
Collaborator

sergiusens commented Feb 26, 2016

retest this please
@sergiusens
Collaborator

sergiusens commented Feb 28, 2016

retest this please
@sergiusens
Collaborator

sergiusens commented Feb 28, 2016

retest this please
examples_tests/__init__.py
- [self.snapcraft_command, 'snap'], cwd=working_dir,
- stderr=subprocess.STDOUT).decode('utf-8')
+ [self.snapcraft_command, '--debug', 'snap'], cwd=working_dir,
+ stderr=subprocess.STDOUT)
@sergiusens

sergiusens Feb 28, 2016

Collaborator

@elopio I'm lost as with what is going on here:

Traceback (most recent call last):
  File "/home/jenkins-slave/workspace/github-snapcraft-examples-tests-cloud/examples_tests/tests.py", line 98, in test_example
    self.build_snap(self.example_dir)
  File "/home/jenkins-slave/workspace/github-snapcraft-examples-tests-cloud/examples_tests/__init__.py", line 129, in build_snap
    stderr=subprocess.STDOUT).decode('utf-8')
  File "/usr/lib/python3.5/subprocess.py", line 626, in check_output
    **kwargs).stdout
  File "/usr/lib/python3.5/subprocess.py", line 708, in run
    output=stdout, stderr=stderr)
@sergiusens
Collaborator

sergiusens commented Feb 28, 2016

retest this please
HACKING.md
+To update the list of libraries that get excluded from inclusion into a
+snap run:
+
+ ./libraries/generate_list.py libraries/<release>
@kyrofa

kyrofa Feb 29, 2016

Member

Should this be generate_lib_list.py?

@sergiusens
Collaborator

sergiusens commented Feb 29, 2016

retest this please
HACKING.md
+
+e.g.; to update the list for 16.04,
+
+ ./libraries/generate_lib_list.py libraries/16.04
@didrocks

didrocks Mar 1, 2016

Contributor

What do you think of having this running as a git release hook? I'm afraid that updates will be sporadic otherwise and ofc, there is no way of getting that running during package build (as accessing external servers).

@kyrofa
Member

kyrofa commented Mar 1, 2016

I'm investigating why this doesn't work for the opencv example.
@kyrofa
Member

kyrofa commented Mar 3, 2016

So the reason this doesn't work with opencv is the same reason is won't work for anything with a lib in a non-standard area (non-standard being defined as anything not placed in LD_LIBRARY_PATH by yaml.py): it doesn't include the ld.so.conf files parsed to determine which libs to include in the LD_LIBRARY_PATH. The libs themselves are included in the snap, but Snapcraft now has no way to know that, for example, the mesa directory needs to be included in LD_LIBRARY_PATH. Perhaps this code can keep track of where the libs are copied, and can tell Snapcraft about them so the LD_LIBRARY_PATH can be modified accordingly. This gets rid of the hacky ld.so.conf stuff as well.
@coveralls

coveralls commented Mar 24, 2016

Coverage Status

Coverage decreased (-0.2%) to 95.34% when pulling 6d9d14c on sergiusens:feature/1549570/ultimate-pkg-config into 2037d9c on ubuntu-core:master.
snapcraft/pluginhandler.py
@@ -399,6 +398,7 @@ def strip(self, force=False):
self.notify_stage('Stripping')
snap_files, snap_dirs = self.migratable_fileset_for('snap')
_migrate_files(snap_files, snap_dirs, self.stagedir, self.snapdir)
+ _copy_dependencies(self.snapdir)
@sergiusens

sergiusens Mar 24, 2016

Collaborator

Needs tracking and library paths used need to be exported to env
hint for @kyrofa

@coveralls

coveralls commented Mar 24, 2016

Coverage Status

Coverage decreased (-0.2%) to 95.674% when pulling f31718f on sergiusens:feature/1549570/ultimate-pkg-config into 88d21b1 on ubuntu-core:master.
snapcraft/pluginhandler.py
@@ -265,7 +262,7 @@ def pull(self, force=False):
package_files = set()
package_directories = set()
if self.code.stage_packages:
- package_files, package_directories = self._setup_stage_packages()
+ self._setup_stage_packages()
self.code.pull()
# Record the files and directories unpacked from the stage packages
@sergiusens

sergiusens Mar 24, 2016

Collaborator

We probably don't need this anymore
hint @kyrofa

snapcraft/pluginhandler.py
+ destination_path = os.path.join(destination, relpath)
+
+ if os.path.isdir(source_path) and not os.path.islink(source_path):
+ os.makedirs(destination_path, exist_ok=True)
@sergiusens

sergiusens Mar 29, 2016

Collaborator

@kyrofa do we need to create these potential empty dirs? The else clause seems will be able to ensure the directories exist and not migrated until build happens which is too late for the python plugins.

@coveralls

coveralls commented Mar 29, 2016

Coverage Status

Coverage decreased (-0.3%) to 95.564% when pulling 5ba5674 on sergiusens:feature/1549570/ultimate-pkg-config into b18e4e9 on ubuntu-core:master.
@sergiusens
Collaborator

sergiusens commented Mar 29, 2016

@elopio given these pass on amd64 can we give this a run on armhf ?
@sergiusens
Collaborator

sergiusens commented Mar 29, 2016

@elopio I'm still seeing _testtools.testresult.real.StringException: output: {{{No module named 'magic'}}}
snapcraft/yaml.py
env += part.env(part.installdir)
env += _runtime_env(stagedir)
env += _runtime_env(part.installdir)
+ env += _runtime_env(part.ubuntu_unpackdir)
@sergiusens

sergiusens Mar 29, 2016

Collaborator

@kyrofa python plugins fail to work, I'm thinking it might be due to this, did it work for you?

@elopio
Member

elopio commented Mar 29, 2016

retest this please
@elopio
Member

elopio commented Mar 29, 2016

retest this please
@sergiusens
Collaborator

sergiusens commented Mar 29, 2016

Thanks @elopio
@sergiusens
Redo pkg-config handling with library collecting 
pkg-config only handled PKG_CONFIG_SYSROOT_DIR as the stage dir
providing many issues when using packages from installdir for a part
and also build-packages from the host.

For a full implementation, elf dependency tracking from the host
to the snap is also implemented here

Add dependency tracking and use it to determine LD_LIBRARY_PATH.

Also unpacked stage-packages into an intermediate area in pull()
and copied them into installdir in build() to make per-step
cleaning work better.

LP: #1549570

Signed-off-by: Sergio Schvezov <sergio.schvezov@ubuntu.com>
Signed-off-by: Kyle Fazzari <kyle@canonical.com>

All checks have passed

4 successful checks
@snappy-m-o
Examples tests — Success 14 tests run, 0 skipped, 0 failed.
Details
@snappy-m-o
autopkgtest — Success No test results found.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage decreased (-0.3%) to 95.634%
Details
4a5de24
@sergiusens
Collaborator

sergiusens commented Mar 29, 2016

@elopio final call is on you 😉
@elopio
Member

elopio commented Mar 29, 2016

flashing...
@sergiusens
Collaborator

sergiusens commented Mar 30, 2016

@elopio how is it doing?
@elopio
Member

elopio commented Mar 30, 2016

@sergiusens slooooow... Running in the snappy classic in rpi2. All unit tests passed. The integration tests have been running for an hour:

(classic)ubuntu@localhost:~/snapcraft$ ./runtests.sh integration
.................[sudo] password for ubuntu:
EEEE.........[sudo] password for ubuntu:
E....[sudo] password for ubuntu:
E..
@sergiusens
Collaborator

sergiusens commented Mar 30, 2016

@elopio hmm, maybe we can fix the sudo issue for armhf this cycle :-)
@elopio
Member

elopio commented Mar 30, 2016

@sergiusens I can take a look at that.
The tests that could run, ran successfully. The tests that need sudo failed.
👍 from me to land this. I'll see if I can get the failed tests to pass. If they uncover an unlikely error, we can still fix it before the release.

@sergiusens sergiusens merged commit 854431f into snapcore:master Mar 30, 2016
4 checks passed

4 checks passed

Examples tests Success 14 tests run, 0 skipped, 0 failed.
Details
autopkgtest Success No test results found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.3%) to 95.634%
Details

@sergiusens sergiusens deleted the sergiusens:feature/1549570/ultimate-pkg-config branch Mar 30, 2016

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

@sergiusens
Merge pull request #335 from sergiusens/feature/1549570/ultimate-pkg-… 
…config

Redo pkg-config handling with library collecting
691dd0a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment