| Message ID | 20250226124720.6462-2-eric.meyers@arthrex.com |
|---|---|
| State | Accepted, archived |
| Commit | 5fa6137b6d98544766f3152b874e67d04fafb88f |
| Headers | show |
| Series | NPM Fetcher Private Registry Authentication Support: | expand |
On Wed, Feb 26, 2025 at 4:47 AM Eric Meyers <eric.meyers15310@gmail.com> wrote: > Signed-off-by: Eric Meyers <eric.meyers@arthrex.com> > Cc: Geoff Parker <geoffrey.parker@arthrex.com> > Cc: Alexander Kanavin <alex.kanavin@gmail.com> > Cc: Chuck Wolber <chuckwolber@gmail.com> > --- > lib/bb/fetch2/npm.py | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/lib/bb/fetch2/npm.py b/lib/bb/fetch2/npm.py > index c09f05044..7486049b8 100644 > --- a/lib/bb/fetch2/npm.py > +++ b/lib/bb/fetch2/npm.py > @@ -107,6 +107,22 @@ class NpmEnvironment(object): > """Run npm command in a controlled environment""" > with tempfile.TemporaryDirectory() as tmpdir: > d = bb.data.createCopy(self.d) > + > + # If the user opts to use their own NPMRC file, then use it > within the NPM environment > + # (along with the user provided configs). Otherwise, leave it > blank. > + if d.getVar("BB_USE_HOME_NPMRC_FILE") == "1": > + home_npmrc_file = os.path.join(os.environ.get("HOME"), > ".npmrc") > + tmp_npmrc_file = os.path.join(tmpdir, ".npmrc") > + bb.warn("BB_USE_HOME_NPMRC_FILE flag is set - NPM fetcher > will now use the .npmrc file in the HOME directory.") > + if os.path.exists(home_npmrc_file): > + bb.utils.copyfile(home_npmrc_file, tmp_npmrc_file) > + # Still need to append the contents of the user > config created in __init__ > + with open(tmp_npmrc_file, 'a') as t: > + with open(self.user_config.name, 'r') as u: > + t.write(u.read()) > + t.flush() > + self.user_config.name = tmp_npmrc_file > + > d.setVar("PATH", d.getVar("PATH")) # PATH might contain > $HOME - evaluate it before patching > d.setVar("HOME", tmpdir) > > Thank you for the patch! I see a few nits. Recommendations further down in this message somewhat obviate this first one, but I am including it anyway for completeness. I think you should combine the two if statements. There does not seem to be any point in printing the bb.warn if there is no ${HOME}/.nmprc file. I like the use of the BB_USE_ prefix, because it vests the policy decision to allow outside influence with bitbake, where it belongs. However I recommend shortening it a bit to BB_USE_HOME_NPMRC. The addition of _FILE seems a little redundant, and it narrows the future options a bit more than necessary. And, given that you are adding a new variable, I think you should include a patch to add that variable to the bitbake manual. Rather than clutter things up in the run() method, you can simplify your change greatly by creating a separate instance method (e.g. __home_npmrc(..)) in the NpmEnvironment class which only returns a file path, or None. It is in that method that you would check for the ${HOME}/.npmrc file, the BB_HOME_NPMRC flag, and do the bb.warn. Then you would return the string that is the file path to the user's .npmrc. No need for any file copy, or renaming. Then in NpmEnvironment __init__ you would do something like this: self.user_config = self.home_npmrc(self, d) if self.user_config is None: self.user_config = tempfile.NamedTemporaryFile(mode="w", buffering=1) for key, value in configs: self.user_config.write("%s=%s\n" % (key, value)) That way you get everything you were looking for without an extra file copy and name swap. **IMPORTANT** There is an obvious bug in my example code that I do not have time to resolve. You need to open the self.user_config file for appending if it is not None. You should do all this in a with block so you do not have to worry about closing file handles. Also worth noting, the npm.bbclass in oe-core uses the configs variable in __init__ for global config, which is then interpreted as user_config. I do not think that is necessarily going to present a problem, but it might be a refactor target down the line (or now?) to make the distinction between user and global config more clear. ..Ch:W..
On Wed, Feb 26, 2025 at 11:14 AM Chuck Wolber <chuckwolber@gmail.com> wrote: > > On Wed, Feb 26, 2025 at 4:47 AM Eric Meyers <eric.meyers15310@gmail.com> > wrote: > >> Signed-off-by: Eric Meyers <eric.meyers@arthrex.com> >> Cc: Geoff Parker <geoffrey.parker@arthrex.com> >> Cc: Alexander Kanavin <alex.kanavin@gmail.com> >> Cc: Chuck Wolber <chuckwolber@gmail.com> >> --- >> lib/bb/fetch2/npm.py | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/lib/bb/fetch2/npm.py b/lib/bb/fetch2/npm.py >> index c09f05044..7486049b8 100644 >> --- a/lib/bb/fetch2/npm.py >> +++ b/lib/bb/fetch2/npm.py >> @@ -107,6 +107,22 @@ class NpmEnvironment(object): >> """Run npm command in a controlled environment""" >> with tempfile.TemporaryDirectory() as tmpdir: >> d = bb.data.createCopy(self.d) >> + >> + # If the user opts to use their own NPMRC file, then use it >> within the NPM environment >> + # (along with the user provided configs). Otherwise, leave >> it blank. >> + if d.getVar("BB_USE_HOME_NPMRC_FILE") == "1": >> + home_npmrc_file = os.path.join(os.environ.get("HOME"), >> ".npmrc") >> + tmp_npmrc_file = os.path.join(tmpdir, ".npmrc") >> + bb.warn("BB_USE_HOME_NPMRC_FILE flag is set - NPM >> fetcher will now use the .npmrc file in the HOME directory.") >> + if os.path.exists(home_npmrc_file): >> + bb.utils.copyfile(home_npmrc_file, tmp_npmrc_file) >> + # Still need to append the contents of the user >> config created in __init__ >> + with open(tmp_npmrc_file, 'a') as t: >> + with open(self.user_config.name, 'r') as u: >> + t.write(u.read()) >> + t.flush() >> + self.user_config.name = tmp_npmrc_file >> + >> d.setVar("PATH", d.getVar("PATH")) # PATH might contain >> $HOME - evaluate it before patching >> d.setVar("HOME", tmpdir) >> >> > > Thank you for the patch! I see a few nits. > > Recommendations further down in this message somewhat obviate this first > one, but I am including it anyway for completeness. I think you should > combine the two if statements. There does not seem to be any point in > printing the bb.warn if there is no ${HOME}/.nmprc file. > > I like the use of the BB_USE_ prefix, because it vests the policy decision > to > allow outside influence with bitbake, where it belongs. However I recommend > shortening it a bit to BB_USE_HOME_NPMRC. The addition of _FILE seems a > little > redundant, and it narrows the future options a bit more than necessary. > > And, given that you are adding a new variable, I think you should include a > patch to add that variable to the bitbake manual. > > Rather than clutter things up in the run() method, you can simplify your > change greatly by creating a separate instance method (e.g. > __home_npmrc(..)) > in the NpmEnvironment class which only returns a file path, or None. It is > in > that method that you would check for the ${HOME}/.npmrc file, the > BB_HOME_NPMRC > flag, and do the bb.warn. Then you would return the string that is the file > path to the user's .npmrc. No need for any file copy, or renaming. > > Then in NpmEnvironment __init__ you would do something like this: > > self.user_config = self.home_npmrc(self, d) > if self.user_config is None: > self.user_config = tempfile.NamedTemporaryFile(mode="w", buffering=1) > for key, value in configs: > self.user_config.write("%s=%s\n" % (key, value)) > Apologies for not proof-reading my code. That example code is obviously not what you would want to do. Hopefully the actual meaning bled through somehow. I meant something closer to this: self.user_config = tempfile.NamedTemporaryFile(mode="w", buffering=1) hn = self.home_npmrc(self, d) if hn is not None: with open(hn, 'r') as hnf: self.user_config.write(hnf.read()) for key, value in configs: self.user_config.write("%s=%s\n" % (key, value)) ..Ch:W..
On Wed, Feb 26, 2025 at 10:49 PM Chuck Wolber <chuckwolber@gmail.com> wrote: > > On Wed, Feb 26, 2025 at 11:14 AM Chuck Wolber <chuckwolber@gmail.com> > wrote: > >> >> Thank you for the patch! I see a few nits. >> >> Recommendations further down in this message somewhat obviate this first >> one, but I am including it anyway for completeness. I think you should >> combine the two if statements. There does not seem to be any point in >> printing the bb.warn if there is no ${HOME}/.nmprc file. >> >> I like the use of the BB_USE_ prefix, because it vests the policy >> decision to >> allow outside influence with bitbake, where it belongs. However I >> recommend >> shortening it a bit to BB_USE_HOME_NPMRC. The addition of _FILE seems a >> little >> redundant, and it narrows the future options a bit more than necessary. >> >> And, given that you are adding a new variable, I think you should include >> a >> patch to add that variable to the bitbake manual. >> >> Rather than clutter things up in the run() method, you can simplify your >> change greatly by creating a separate instance method (e.g. >> __home_npmrc(..)) >> in the NpmEnvironment class which only returns a file path, or None. It >> is in >> that method that you would check for the ${HOME}/.npmrc file, the >> BB_HOME_NPMRC >> flag, and do the bb.warn. Then you would return the string that is the >> file >> path to the user's .npmrc. No need for any file copy, or renaming. >> >> Then in NpmEnvironment __init__ you would do something like this: >> >> self.user_config = self.home_npmrc(self, d) >> if self.user_config is None: >> self.user_config = tempfile.NamedTemporaryFile(mode="w", buffering=1) >> for key, value in configs: >> self.user_config.write("%s=%s\n" % (key, value)) >> > > > Apologies for not proof-reading my code. That example code is obviously not > what you would want to do. Hopefully the actual meaning bled through > somehow. > > I meant something closer to this: > > self.user_config = tempfile.NamedTemporaryFile(mode="w", buffering=1) > hn = self.home_npmrc(self, d) > if hn is not None: > with open(hn, 'r') as hnf: > self.user_config.write(hnf.read()) > for key, value in configs: > self.user_config.write("%s=%s\n" % (key, value)) > > > ..Ch:W.. > Chuck, Thanks a ton for the feedback - much appreciated. I will take some time to digest this, do the rework, and eventually submit a v4 patch. Probably sometime early next week. Thanks!, -Eric
diff --git a/lib/bb/fetch2/npm.py b/lib/bb/fetch2/npm.py index c09f05044..7486049b8 100644 --- a/lib/bb/fetch2/npm.py +++ b/lib/bb/fetch2/npm.py @@ -107,6 +107,22 @@ class NpmEnvironment(object): """Run npm command in a controlled environment""" with tempfile.TemporaryDirectory() as tmpdir: d = bb.data.createCopy(self.d) + + # If the user opts to use their own NPMRC file, then use it within the NPM environment + # (along with the user provided configs). Otherwise, leave it blank. + if d.getVar("BB_USE_HOME_NPMRC_FILE") == "1": + home_npmrc_file = os.path.join(os.environ.get("HOME"), ".npmrc") + tmp_npmrc_file = os.path.join(tmpdir, ".npmrc") + bb.warn("BB_USE_HOME_NPMRC_FILE flag is set - NPM fetcher will now use the .npmrc file in the HOME directory.") + if os.path.exists(home_npmrc_file): + bb.utils.copyfile(home_npmrc_file, tmp_npmrc_file) + # Still need to append the contents of the user config created in __init__ + with open(tmp_npmrc_file, 'a') as t: + with open(self.user_config.name, 'r') as u: + t.write(u.read()) + t.flush() + self.user_config.name = tmp_npmrc_file + d.setVar("PATH", d.getVar("PATH")) # PATH might contain $HOME - evaluate it before patching d.setVar("HOME", tmpdir)
Signed-off-by: Eric Meyers <eric.meyers@arthrex.com> Cc: Geoff Parker <geoffrey.parker@arthrex.com> Cc: Alexander Kanavin <alex.kanavin@gmail.com> Cc: Chuck Wolber <chuckwolber@gmail.com> --- lib/bb/fetch2/npm.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)