Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Alter prefix for pkg-config to get correct values #620
Conversation
kyrofa
reviewed
Jul 1, 2016
| +def fix_pkg_config(root, pkg_config_file): | ||
| + """Opens a pkg_config_file and prefixes the prefix with root.""" | ||
| + pattern = re.compile('^prefix=(?P<prefix>.*)') | ||
| + for line in fileinput.input(pkg_config_file, inplace=True): |
kyrofa
Jul 1, 2016
•
Edited 1 time
-
kyrofa
Jul 1, 2016
Member
-
Jul 1, 2016
Ah, I've not seen this before. We've got some other file replacement functions that might be able to use this.
kyrofa
reviewed
Jul 1, 2016
| @@ -644,7 +651,7 @@ def _migratable_filesets(fileset, srcdir): | ||
| def _migrate_files(snap_files, snap_dirs, srcdir, dstdir, missing_ok=False, | ||
| - follow_symlinks=False): | ||
| + follow_symlinks=False, fixup_func=None): |
kyrofa
Jul 1, 2016
Member
This doesn't make for a particularly intuitive function from its signature, but I guess there's little reason to walk dstdir again.
kyrofa
Jul 1, 2016
•
Edited 1 time
-
kyrofa
Jul 1, 2016
Member
-
Jul 1, 2016
Nah, I take that back-- could see this being useful. I wonder if it's worth supporting multiple fixup_funcs.
didrocks
Jul 1, 2016
Contributor
Don't you want to make it a lambda function taking one parameter? That way, it's clear from the signature that the function is of form:
def fun(dst):
pass
As you don't have docstrings to specify it.
|
Yeah this is looking good. Missing a rewrite for the installdir (as you mentioned)-- also could use some tests |
|
El 01/07/16 a las 09:23, Kyle Fazzari escribió:
Test will come when I figure out the installdir story :-) |
didrocks
reviewed
Jul 1, 2016
| @@ -664,6 +671,8 @@ def _migrate_files(snap_files, snap_dirs, srcdir, dstdir, missing_ok=False, | ||
| os.remove(dst) | ||
| common.link_or_copy(src, dst, follow_symlinks=follow_symlinks) | ||
| + if fixup_func: |
didrocks
Jul 1, 2016
Contributor
and so, with a lambda, you can just call the function and not caring :)
didrocks
reviewed
Jul 1, 2016
| + pattern_trim = re.compile( | ||
| + '^prefix={}(?P<prefix>.*)'.format(prefix_trim)) | ||
| + pattern = re.compile('^prefix=(?P<prefix>.*)') | ||
| + for line in fileinput.input(pkg_config_file, inplace=True): |
didrocks
Jul 1, 2016
Contributor
I personnally tend to use fileinput as a contextmanager (easier to read, cleaner on opening/close files) as it's a more pythonish way of doing files handling in python3: https://docs.python.org/3/library/fileinput.html
didrocks
reviewed
Jul 1, 2016
| elif os.path.exists(path): | ||
| _fix_filemode(path) | ||
| + if path.endswith('.pc') and not os.path.islink(path): | ||
| + fix_pkg_config(debdir, path) |
didrocks
Jul 1, 2016
Contributor
Shouldn't you call fixup_func() rather here? (need to match the number of parameters) and move it to common as you are duplicating the checks for pc files + symlinks?
sergiusens
Jul 1, 2016
Collaborator
This is the only comment I have not addressed as I really want to get rid of common
didrocks
reviewed
Jul 1, 2016
| + logger.debug('Skipping {}'.format(target)) | ||
| + return | ||
| + if not os.path.exists(target): | ||
| + if not _try_copy_local(path, target): |
sergiusens
added some commits
Jul 1, 2016
|
the java example failed to build in examples tests. Unrelated IMO and it passed in adt. |
|
Yeah, this looks very good |
|
tested on @mhall119 's pantheon and works fine |
sergiusens
merged commit f25abf9
into
snapcore:master
Jul 1, 2016
sergiusens
deleted the
sergiusens:bugfix/1595243/pkg-config-prefix-what
branch
Jul 1, 2016
pushed a commit
to kalikiana/snapcraft
that referenced
this pull request
Apr 6, 2017
sergiusens commentedJun 30, 2016
LP: #1595243
Signed-off-by: Sergio Schvezov sergio.schvezov@ubuntu.com