Skip to contentSkip to conversation

npm fetcher fixes

PATCH New Paul Eggleton 1 participant 7 messages Sep 5, 2016

lore
Share & tools
lore
#0 lore raw

cover npm fetcher fixes

Some fixes for issues in the npm fetcher that I found while trying to implement support for non-npm node.js source trees.

The following changes since commit dc756510a95f88b192352be6fcd1d5d77852c348:

bb.fetch2.svn: correctly pass workdir when fetching (2016-09-02 16:29:36 +0100)

are available in the git repository at:

git://git.yoctoproject.org/poky-contrib paule/bb-npm-fixes3
http://git.yoctoproject.org/cgit.cgi/poky-contrib/log/?h=paule/bb-npm-fixes3

Paul Eggleton (6):
fetch2/npm: explicitly specify workdir
fetch2/npm: fix broken fetches if more than one npm URL fetched fetch2/npm: support subdir= parameter
fetch2/npm: handle top-level shrinkwrap file fetch2/npm: clarify comment
lib/bb/utils: edit_metadata() comment tweaks

 lib/bb/fetch2/npm.py | 29 +++++++++++++++++++++--------
 lib/bb/utils.py      |  4 +++-
 2 files changed, 24 insertions(+), 9 deletions(-)
Signature · 1 line
2.5.5
#1 lore raw

PATCH 1/6 fetch2/npm: explicitly specify workdir

We were downloading into the current directory here, which is fine if that current directory can be expected to be the right place - but that's not true when called from recipetool within OE. We should explicitly specify the directory to run the command in and then there won't be a problem.

---
 lib/bb/fetch2/npm.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
lib/bb/fetch2/npm.py +2 -1
index b26ac22..65ccb0d 100644
@@ -102,7 +102,8 @@ class Npm(FetchMethod):
     def _runwget(self, ud, d, command, quiet):
         logger.debug(2, "Fetching %s using command '%s'" % (ud.url, command))
         bb.fetch2.check_network_access(d, command)
-        runfetchcmd(command, d, quiet)
+        dldir = d.getVar("DL_DIR", True)
+        runfetchcmd(command, d, quiet, workdir=dldir)
 
     def _unpackdep(self, ud, pkg, data, destdir, dldir, d):
         file = data[pkg]['tgz']
Signature · 1 line
2.5.5
#2 lore raw

PATCH 2/6 fetch2/npm: fix broken fetches if more than one npm URL fetched

In reply to Paul Eggleton

You cannot set a URL-specific value in an object-level variable on the FetchMethod in urldata_init() or the result is the value specific to the last URL will be the one that gets set. This prevented fetching more than one npm:// URL correctly - the other tarballs would not download to the correct location and do_unpack failed to find them as a result.

Fix required in order to support [YOCTO #9537].

---
 lib/bb/fetch2/npm.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
lib/bb/fetch2/npm.py +3 -3
index 65ccb0d..9b2d9e1 100644
@@ -88,7 +88,7 @@ class Npm(FetchMethod):
         ud.localpath = d.expand("${DL_DIR}/npm/%s" % ud.bbnpmmanifest)
 
         self.basecmd = d.getVar("FETCHCMD_wget", True) or "/usr/bin/env wget -O -t 2 -T 30 -nv --passive-ftp --no-check-certificate "
-        self.basecmd += " --directory-prefix=%s " % prefixdir
+        ud.prefixdir = prefixdir
 
         ud.write_tarballs = ((data.getVar("BB_GENERATE_MIRROR_TARBALLS", d, True) or "0") != "0")
         ud.mirrortarball = 'npm_%s-%s.tar.xz' % (ud.pkgname, ud.version)
@@ -182,7 +182,7 @@ class Npm(FetchMethod):
         outputurl = pdata['dist']['tarball']
         data[pkg] = {}
         data[pkg]['tgz'] = os.path.basename(outputurl)
-        self._runwget(ud, d, "%s %s" % (self.basecmd, outputurl), False)
+        self._runwget(ud, d, "%s --directory-prefix=%s %s" % (self.basecmd, ud.prefixdir, outputurl), False)
 
         dependencies = pdata.get('dependencies', {})
         optionalDependencies = pdata.get('optionalDependencies', {})
@@ -209,7 +209,7 @@ class Npm(FetchMethod):
             outputurl = runfetchcmd(fetchcmd, d, True)
         else:
             outputurl = data['resolved']
-        self._runwget(ud, d, "%s %s" % (self.basecmd, outputurl), False)
+        self._runwget(ud, d, "%s --directory-prefix=%s %s" % (self.basecmd, ud.prefixdir, outputurl), False)
         manifest[pkg] = {}
         manifest[pkg]['tgz'] = os.path.basename(outputurl).rstrip()
         manifest[pkg]['deps'] = {}
Signature · 1 line
2.5.5
#3 lore raw

PATCH 3/6 fetch2/npm: support subdir= parameter

In reply to Paul Eggleton

"npmpkg" can be a default, but it should respect the subdir parameter as with other FetchMethods. This allows us to have more than one npm:// entry in SRC_URI without nasty hacks.

Fix required in order to support [YOCTO #9537].

---
 lib/bb/fetch2/npm.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
lib/bb/fetch2/npm.py +6 -1
index 9b2d9e1..134bd35 100644
@@ -138,7 +138,12 @@ class Npm(FetchMethod):
             workobj = json.load(datafile)
         dldir = "%s/%s" % (os.path.dirname(ud.localpath), ud.pkgname)
 
-        self._unpackdep(ud, ud.pkgname, workobj,  "%s/npmpkg" % destdir, dldir, d)
+        if 'subdir' in ud.parm:
+            unpackdir = '%s/%s' % (destdir, ud.parm.get('subdir'))
+        else:
+            unpackdir = '%s/npmpkg' % destdir
+
+        self._unpackdep(ud, ud.pkgname, workobj, unpackdir, dldir, d)
 
     def _parse_view(self, output):
         '''
Signature · 1 line
2.5.5
#4 lore raw

PATCH 4/6 fetch2/npm: handle top-level shrinkwrap file

In reply to Paul Eggleton

Allow using a top-level shrinkwrap file with one or more npm:// dependencies, i.e. if the module isn't found at the top level then look one level down.

Part of the fix for [YOCTO #9537].

---
 lib/bb/fetch2/npm.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
lib/bb/fetch2/npm.py +9 -2
index 134bd35..08d6876 100644
@@ -204,8 +204,15 @@ class Npm(FetchMethod):
         for dep, version in depsfound.items():
             self._getdependencies(dep, data[pkg]['deps'], version, d, ud)
 
-    def _getshrinkeddependencies(self, pkg, data, version, d, ud, lockdown, manifest):
+    def _getshrinkeddependencies(self, pkg, data, version, d, ud, lockdown, manifest, toplevel=True):
         logger.debug(2, "NPM shrinkwrap file is %s" % data)
+        if toplevel:
+            name = data.get('name', None)
+            if name and name != pkg:
+                for obj in data.get('dependencies', []):
+                    if obj == pkg:
+                        self._getshrinkeddependencies(obj, data['dependencies'][obj], data['dependencies'][obj]['version'], d, ud, lockdown, manifest, False)
+                        return
         outputurl = "invalid"
         if ('resolved' not in data) or (not data['resolved'].startswith('http')):
             # will be the case for ${PN}
@@ -231,7 +238,7 @@ class Npm(FetchMethod):
         if 'dependencies' in data:
             for obj in data['dependencies']:
                 logger.debug(2, "Found dep is %s" % str(obj))
-                self._getshrinkeddependencies(obj, data['dependencies'][obj], data['dependencies'][obj]['version'], d, ud, lockdown, manifest[pkg]['deps'])
+                self._getshrinkeddependencies(obj, data['dependencies'][obj], data['dependencies'][obj]['version'], d, ud, lockdown, manifest[pkg]['deps'], False)
 
     def download(self, ud, d):
         """Fetch url"""
Signature · 1 line
2.5.5
#5 lore raw

PATCH 5/6 fetch2/npm: clarify comment

In reply to Paul Eggleton

The correct name of the parameter is "version" not "ver" so ensure we aren't misleading the user by giving the latter in an example.

---
 lib/bb/fetch2/npm.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
lib/bb/fetch2/npm.py +1 -1
index 08d6876..43929ce 100644
@@ -13,7 +13,7 @@ Usage in the recipe:
     - name
     - version
 
-    npm://registry.npmjs.org/${PN}/-/${PN}-${PV}.tgz  would become npm://registry.npmjs.org;name=${PN};ver=${PV}
+    npm://registry.npmjs.org/${PN}/-/${PN}-${PV}.tgz  would become npm://registry.npmjs.org;name=${PN};version=${PV}
     The fetcher all triggers off the existence of ud.localpath. If that exists and has the ".done" stamp, its assumed the fetch is good/done
 
 """
Signature · 1 line
2.5.5
#6 lore raw

PATCH 6/6 lib/bb/utils: edit_metadata() comment tweaks

In reply to Paul Eggleton

No functional changes, just make a couple of minor tweaks to the comments for edit_metadata():

* There are four elements to be returned by the callback function
* Add an example return statement for when you don't want to modify the value

---
 lib/bb/utils.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
lib/bb/utils.py +3 -1
index a855d58..729848a 100644
@@ -1081,7 +1081,7 @@ def edit_metadata(meta_lines, variables, varfunc, match_overrides=False):
                 newlines: list of lines up to this point. You can use
                     this to prepend lines before this variable setting
                     if you wish.
-            and should return a three-element tuple:
+            and should return a four-element tuple:
                 newvalue: new value to substitute in, or None to drop
                     the variable setting entirely. (If the removal
                     results in two consecutive blank lines, one of the
@@ -1095,6 +1095,8 @@ def edit_metadata(meta_lines, variables, varfunc, match_overrides=False):
                     multi-line value to continue on the same line as
                     the assignment, False to indent before the first
                     element.
+            To clarify, if you wish not to change the value, then you
+            would return like this: return origvalue, None, 0, True
         match_overrides: True to match items with _overrides on the end,
             False otherwise
     Returns a tuple:
Signature · 1 line
2.5.5