You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by "nanonyme (via GitHub)" <gi...@apache.org> on 2023/01/29 15:50:52 UTC
[GitHub] [buildstream] nanonyme opened a new pull request, #1820: Stop passing URL opener across process boundaries
nanonyme opened a new pull request, #1820:
URL: https://github.com/apache/buildstream/pull/1820
Should fix https://github.com/apache/buildstream/issues/1766
The object is not picklable. This cannot be supported. Instead create URL opener when needed in child process. Drops URL opener caching which may result in minor performance impact.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on pull request #1820: Stop passing URL opener across process boundaries
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#issuecomment-1407713160
I suspect this is CI-only problem. Environment is working in unexpected way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on a diff in pull request #1820: Stop passing URL opener across process boundaries
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on code in PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#discussion_r1103643600
##########
src/buildstream/downloadablefilesource.py:
##########
@@ -260,29 +260,29 @@ def _get_mirror_file(self, sha=None):
return self.__default_mirror_file
- @classmethod
- def _reset_url_opener(cls):
- # Needed for tests, in order to cleanup the `netrc` configuration.
- cls.__urlopener = None # pylint: disable=unused-private-member
-
- def __get_urlopener(self):
- if not DownloadableFileSource.__urlopener:
- try:
- netrc_config = netrc.netrc()
- except OSError:
- # If the .netrc file was not found, FileNotFoundError will be
- # raised, but OSError will be raised directly by the netrc package
- # in the case that $HOME is not set.
- #
- # This will catch both cases.
- #
- DownloadableFileSource.__urlopener = urllib.request.build_opener()
- except netrc.NetrcParseError as e:
- self.warn("{}: While reading .netrc: {}".format(self, e))
- return urllib.request.build_opener()
- else:
- netrc_pw_mgr = _NetrcPasswordManager(netrc_config)
- http_auth = urllib.request.HTTPBasicAuthHandler(netrc_pw_mgr)
- ftp_handler = _NetrcFTPOpener(netrc_config)
- DownloadableFileSource.__urlopener = urllib.request.build_opener(http_auth, ftp_handler)
- return DownloadableFileSource.__urlopener
+
+class _UrlOpenerCreator:
+ def __init__(self):
+ self.netrc_config = None
+
+ try:
+ self.netrc_config = netrc.netrc()
+ except OSError:
+ # If the .netrc file was not found, FileNotFoundError will be
+ # raised, but OSError will be raised directly by the netrc package
+ # in the case that $HOME is not set.
+ #
+ # This will catch both cases.
+ pass
+ except netrc.NetrcParseError:
+ # netrc file exists but is invalid, passthrough since we can't log here
Review Comment:
I decoupled netrc parsing into a method in DownloadableFileSource since it was anyway parsed in main process so now it has safely access to logging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on a diff in pull request #1820: Stop passing URL opener across process boundaries
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on code in PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#discussion_r1103600836
##########
src/buildstream/downloadablefilesource.py:
##########
@@ -260,29 +260,29 @@ def _get_mirror_file(self, sha=None):
return self.__default_mirror_file
- @classmethod
- def _reset_url_opener(cls):
- # Needed for tests, in order to cleanup the `netrc` configuration.
- cls.__urlopener = None # pylint: disable=unused-private-member
-
- def __get_urlopener(self):
- if not DownloadableFileSource.__urlopener:
- try:
- netrc_config = netrc.netrc()
- except OSError:
- # If the .netrc file was not found, FileNotFoundError will be
- # raised, but OSError will be raised directly by the netrc package
- # in the case that $HOME is not set.
- #
- # This will catch both cases.
- #
- DownloadableFileSource.__urlopener = urllib.request.build_opener()
- except netrc.NetrcParseError as e:
- self.warn("{}: While reading .netrc: {}".format(self, e))
- return urllib.request.build_opener()
- else:
- netrc_pw_mgr = _NetrcPasswordManager(netrc_config)
- http_auth = urllib.request.HTTPBasicAuthHandler(netrc_pw_mgr)
- ftp_handler = _NetrcFTPOpener(netrc_config)
- DownloadableFileSource.__urlopener = urllib.request.build_opener(http_auth, ftp_handler)
- return DownloadableFileSource.__urlopener
+
+class _UrlOpenerCreator:
+ def __init__(self):
+ self.netrc_config = None
+
+ try:
+ self.netrc_config = netrc.netrc()
+ except OSError:
+ # If the .netrc file was not found, FileNotFoundError will be
+ # raised, but OSError will be raised directly by the netrc package
+ # in the case that $HOME is not set.
+ #
+ # This will catch both cases.
+ pass
+ except netrc.NetrcParseError:
+ # netrc file exists but is invalid, passthrough since we can't log here
Review Comment:
Not unless we make this fatal
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on pull request #1820: Stop passing URL opener across process boundaries
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#issuecomment-1427114060
It definitely looks like a race condition with multiple parties trying to write same file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] BenjaminSchubert commented on a diff in pull request #1820: Stop passing URL opener across process boundaries
Posted by "BenjaminSchubert (via GitHub)" <gi...@apache.org>.
BenjaminSchubert commented on code in PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#discussion_r1103599143
##########
src/buildstream/downloadablefilesource.py:
##########
@@ -260,29 +260,29 @@ def _get_mirror_file(self, sha=None):
return self.__default_mirror_file
- @classmethod
- def _reset_url_opener(cls):
- # Needed for tests, in order to cleanup the `netrc` configuration.
- cls.__urlopener = None # pylint: disable=unused-private-member
-
- def __get_urlopener(self):
- if not DownloadableFileSource.__urlopener:
- try:
- netrc_config = netrc.netrc()
- except OSError:
- # If the .netrc file was not found, FileNotFoundError will be
- # raised, but OSError will be raised directly by the netrc package
- # in the case that $HOME is not set.
- #
- # This will catch both cases.
- #
- DownloadableFileSource.__urlopener = urllib.request.build_opener()
- except netrc.NetrcParseError as e:
- self.warn("{}: While reading .netrc: {}".format(self, e))
- return urllib.request.build_opener()
- else:
- netrc_pw_mgr = _NetrcPasswordManager(netrc_config)
- http_auth = urllib.request.HTTPBasicAuthHandler(netrc_pw_mgr)
- ftp_handler = _NetrcFTPOpener(netrc_config)
- DownloadableFileSource.__urlopener = urllib.request.build_opener(http_auth, ftp_handler)
- return DownloadableFileSource.__urlopener
+
+class _UrlOpenerCreator:
+ def __init__(self):
+ self.netrc_config = None
+
+ try:
+ self.netrc_config = netrc.netrc()
+ except OSError:
+ # If the .netrc file was not found, FileNotFoundError will be
+ # raised, but OSError will be raised directly by the netrc package
+ # in the case that $HOME is not set.
+ #
+ # This will catch both cases.
+ pass
+ except netrc.NetrcParseError:
+ # netrc file exists but is invalid, passthrough since we can't log here
+ pass
+
+ def get_url_opener(self):
+ if self.netrc_config:
+ netrc_pw_mgr = _NetrcPasswordManager(self.netrc_config)
+ http_auth = urllib.request.HTTPBasicAuthHandler(netrc_pw_mgr)
+ ftp_handler = _NetrcFTPOpener(self.netrc_config)
+ return urllib.request.build_opener(http_auth, ftp_handler)
+ else:
+ return urllib.request.build_opener()
Review Comment:
A small nitpick but I think
```suggestion
return urllib.request.build_opener()
```
would be a tad easier to read and is more in line with what we do in other places
##########
src/buildstream/downloadablefilesource.py:
##########
@@ -260,29 +260,29 @@ def _get_mirror_file(self, sha=None):
return self.__default_mirror_file
- @classmethod
- def _reset_url_opener(cls):
- # Needed for tests, in order to cleanup the `netrc` configuration.
- cls.__urlopener = None # pylint: disable=unused-private-member
-
- def __get_urlopener(self):
- if not DownloadableFileSource.__urlopener:
- try:
- netrc_config = netrc.netrc()
- except OSError:
- # If the .netrc file was not found, FileNotFoundError will be
- # raised, but OSError will be raised directly by the netrc package
- # in the case that $HOME is not set.
- #
- # This will catch both cases.
- #
- DownloadableFileSource.__urlopener = urllib.request.build_opener()
- except netrc.NetrcParseError as e:
- self.warn("{}: While reading .netrc: {}".format(self, e))
- return urllib.request.build_opener()
- else:
- netrc_pw_mgr = _NetrcPasswordManager(netrc_config)
- http_auth = urllib.request.HTTPBasicAuthHandler(netrc_pw_mgr)
- ftp_handler = _NetrcFTPOpener(netrc_config)
- DownloadableFileSource.__urlopener = urllib.request.build_opener(http_auth, ftp_handler)
- return DownloadableFileSource.__urlopener
+
+class _UrlOpenerCreator:
+ def __init__(self):
+ self.netrc_config = None
+
+ try:
+ self.netrc_config = netrc.netrc()
+ except OSError:
+ # If the .netrc file was not found, FileNotFoundError will be
+ # raised, but OSError will be raised directly by the netrc package
+ # in the case that $HOME is not set.
+ #
+ # This will catch both cases.
+ pass
+ except netrc.NetrcParseError:
+ # netrc file exists but is invalid, passthrough since we can't log here
Review Comment:
Don't we have any way of reporting this? It might become very hard to debug for users if they have a typo in it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on pull request #1820: Stop passing URL opener across process boundaries
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#issuecomment-1434309147
Rebased this and fixed conflicts with downloadablefilesource.py, this PR is now replaced by #1826
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on pull request #1820: Stop passing URL opener across process boundaries
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#issuecomment-1407712640
Looks like this breaks netrfc functionality. Need to debug.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on pull request #1820: Stop passing URL opener across process boundaries
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#issuecomment-1427113646
It looks strange. Apparently the error originates here https://github.com/apache/buildstream/blob/master/src/buildstream/utils.py#L432 and it looks like bst should upon file existing try to delete it and then try to link again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan closed pull request #1820: Stop passing URL opener across process boundaries
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan closed pull request #1820: Stop passing URL opener across process boundaries
URL: https://github.com/apache/buildstream/pull/1820
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on pull request #1820: Stop passing URL opener across process boundaries
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#issuecomment-1427111975
> Failing test appears to have been failed at [...]
Which seems to be the same as https://github.com/apache/buildstream/pull/1814. I guess that's just a flaky test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on pull request #1820: Stop passing URL opener across process boundaries
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#issuecomment-1426807683
Failing test appears to have been failed at ` [00:00:00] FAILURE [4a47c98a] build-shell/buildtree.bst: Failed to link '/home/testuser/buildstream/.tox/py310/tmp/test_shell_pull_cached_buildtr2/cache/artifacts/refs/test/build-shell-buildtree/4a47c98a10df39e65e99d471f96edc5b58d4ea5b9b1f221d0be832a8124b8099 -> /home/testuser/buildstream/.tox/py310/tmp/test_shell_pull_cached_buildtr2/cache/artifacts/refs/test/build-shell-buildtree/38b730aec720dbddba843c9e3ce1ad8073e39d16cc4d25f752bc89d867fbaec1': [Errno 17] File exists: '/home/testuser/buildstream/.tox/py310/tmp/test_shell_pull_cached_buildtr2/cache/artifacts/refs/test/build-shell-buildtree/4a47c98a10df39e65e99d471f96edc5b58d4ea5b9b1f221d0be832a8124b8099' -> '/home/testuser/buildstream/.tox/py310/tmp/test_shell_pull_cached_buildtr2/cache/artifacts/refs/test/build-shell-buildtree/38b730aec720dbddba843c9e3ce1ad8073e39d16cc4d25f752bc89d867fbaec1'`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on a diff in pull request #1820: Stop passing URL opener across process boundaries
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on code in PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#discussion_r1103642686
##########
src/buildstream/downloadablefilesource.py:
##########
@@ -260,29 +260,29 @@ def _get_mirror_file(self, sha=None):
return self.__default_mirror_file
- @classmethod
- def _reset_url_opener(cls):
- # Needed for tests, in order to cleanup the `netrc` configuration.
- cls.__urlopener = None # pylint: disable=unused-private-member
-
- def __get_urlopener(self):
- if not DownloadableFileSource.__urlopener:
- try:
- netrc_config = netrc.netrc()
- except OSError:
- # If the .netrc file was not found, FileNotFoundError will be
- # raised, but OSError will be raised directly by the netrc package
- # in the case that $HOME is not set.
- #
- # This will catch both cases.
- #
- DownloadableFileSource.__urlopener = urllib.request.build_opener()
- except netrc.NetrcParseError as e:
- self.warn("{}: While reading .netrc: {}".format(self, e))
- return urllib.request.build_opener()
- else:
- netrc_pw_mgr = _NetrcPasswordManager(netrc_config)
- http_auth = urllib.request.HTTPBasicAuthHandler(netrc_pw_mgr)
- ftp_handler = _NetrcFTPOpener(netrc_config)
- DownloadableFileSource.__urlopener = urllib.request.build_opener(http_auth, ftp_handler)
- return DownloadableFileSource.__urlopener
+
+class _UrlOpenerCreator:
+ def __init__(self):
+ self.netrc_config = None
+
+ try:
+ self.netrc_config = netrc.netrc()
+ except OSError:
+ # If the .netrc file was not found, FileNotFoundError will be
+ # raised, but OSError will be raised directly by the netrc package
+ # in the case that $HOME is not set.
+ #
+ # This will catch both cases.
+ pass
+ except netrc.NetrcParseError:
+ # netrc file exists but is invalid, passthrough since we can't log here
Review Comment:
Wait, actually, that was not quite true. netrc processing happens in main project side. I will refactor this. still a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on pull request #1820: Stop passing URL opener across process boundaries
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#issuecomment-1427143505
The should probably be a follow-up issue on that. I suspect there might be something specific to test runner that results in this since I haven't ever seen this in real world.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on a diff in pull request #1820: Stop passing URL opener across process boundaries
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on code in PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#discussion_r1103600630
##########
src/buildstream/downloadablefilesource.py:
##########
@@ -260,29 +260,29 @@ def _get_mirror_file(self, sha=None):
return self.__default_mirror_file
- @classmethod
- def _reset_url_opener(cls):
- # Needed for tests, in order to cleanup the `netrc` configuration.
- cls.__urlopener = None # pylint: disable=unused-private-member
-
- def __get_urlopener(self):
- if not DownloadableFileSource.__urlopener:
- try:
- netrc_config = netrc.netrc()
- except OSError:
- # If the .netrc file was not found, FileNotFoundError will be
- # raised, but OSError will be raised directly by the netrc package
- # in the case that $HOME is not set.
- #
- # This will catch both cases.
- #
- DownloadableFileSource.__urlopener = urllib.request.build_opener()
- except netrc.NetrcParseError as e:
- self.warn("{}: While reading .netrc: {}".format(self, e))
- return urllib.request.build_opener()
- else:
- netrc_pw_mgr = _NetrcPasswordManager(netrc_config)
- http_auth = urllib.request.HTTPBasicAuthHandler(netrc_pw_mgr)
- ftp_handler = _NetrcFTPOpener(netrc_config)
- DownloadableFileSource.__urlopener = urllib.request.build_opener(http_auth, ftp_handler)
- return DownloadableFileSource.__urlopener
+
+class _UrlOpenerCreator:
+ def __init__(self):
+ self.netrc_config = None
+
+ try:
+ self.netrc_config = netrc.netrc()
+ except OSError:
+ # If the .netrc file was not found, FileNotFoundError will be
+ # raised, but OSError will be raised directly by the netrc package
+ # in the case that $HOME is not set.
+ #
+ # This will catch both cases.
+ pass
+ except netrc.NetrcParseError:
+ # netrc file exists but is invalid, passthrough since we can't log here
+ pass
+
+ def get_url_opener(self):
+ if self.netrc_config:
+ netrc_pw_mgr = _NetrcPasswordManager(self.netrc_config)
+ http_auth = urllib.request.HTTPBasicAuthHandler(netrc_pw_mgr)
+ ftp_handler = _NetrcFTPOpener(self.netrc_config)
+ return urllib.request.build_opener(http_auth, ftp_handler)
+ else:
+ return urllib.request.build_opener()
Review Comment:
The problem is we no longer have access to source here so we can't call BuildStream logging primitives. It never worked anyway
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on a diff in pull request #1820: Stop passing URL opener across process boundaries
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on code in PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#discussion_r1103600630
##########
src/buildstream/downloadablefilesource.py:
##########
@@ -260,29 +260,29 @@ def _get_mirror_file(self, sha=None):
return self.__default_mirror_file
- @classmethod
- def _reset_url_opener(cls):
- # Needed for tests, in order to cleanup the `netrc` configuration.
- cls.__urlopener = None # pylint: disable=unused-private-member
-
- def __get_urlopener(self):
- if not DownloadableFileSource.__urlopener:
- try:
- netrc_config = netrc.netrc()
- except OSError:
- # If the .netrc file was not found, FileNotFoundError will be
- # raised, but OSError will be raised directly by the netrc package
- # in the case that $HOME is not set.
- #
- # This will catch both cases.
- #
- DownloadableFileSource.__urlopener = urllib.request.build_opener()
- except netrc.NetrcParseError as e:
- self.warn("{}: While reading .netrc: {}".format(self, e))
- return urllib.request.build_opener()
- else:
- netrc_pw_mgr = _NetrcPasswordManager(netrc_config)
- http_auth = urllib.request.HTTPBasicAuthHandler(netrc_pw_mgr)
- ftp_handler = _NetrcFTPOpener(netrc_config)
- DownloadableFileSource.__urlopener = urllib.request.build_opener(http_auth, ftp_handler)
- return DownloadableFileSource.__urlopener
+
+class _UrlOpenerCreator:
+ def __init__(self):
+ self.netrc_config = None
+
+ try:
+ self.netrc_config = netrc.netrc()
+ except OSError:
+ # If the .netrc file was not found, FileNotFoundError will be
+ # raised, but OSError will be raised directly by the netrc package
+ # in the case that $HOME is not set.
+ #
+ # This will catch both cases.
+ pass
+ except netrc.NetrcParseError:
+ # netrc file exists but is invalid, passthrough since we can't log here
+ pass
+
+ def get_url_opener(self):
+ if self.netrc_config:
+ netrc_pw_mgr = _NetrcPasswordManager(self.netrc_config)
+ http_auth = urllib.request.HTTPBasicAuthHandler(netrc_pw_mgr)
+ ftp_handler = _NetrcFTPOpener(self.netrc_config)
+ return urllib.request.build_opener(http_auth, ftp_handler)
+ else:
+ return urllib.request.build_opener()
Review Comment:
The problem is we no longer have access to source here so we can't call BuildStream logging primitives. It never worked anyway
##########
src/buildstream/downloadablefilesource.py:
##########
@@ -260,29 +260,29 @@ def _get_mirror_file(self, sha=None):
return self.__default_mirror_file
- @classmethod
- def _reset_url_opener(cls):
- # Needed for tests, in order to cleanup the `netrc` configuration.
- cls.__urlopener = None # pylint: disable=unused-private-member
-
- def __get_urlopener(self):
- if not DownloadableFileSource.__urlopener:
- try:
- netrc_config = netrc.netrc()
- except OSError:
- # If the .netrc file was not found, FileNotFoundError will be
- # raised, but OSError will be raised directly by the netrc package
- # in the case that $HOME is not set.
- #
- # This will catch both cases.
- #
- DownloadableFileSource.__urlopener = urllib.request.build_opener()
- except netrc.NetrcParseError as e:
- self.warn("{}: While reading .netrc: {}".format(self, e))
- return urllib.request.build_opener()
- else:
- netrc_pw_mgr = _NetrcPasswordManager(netrc_config)
- http_auth = urllib.request.HTTPBasicAuthHandler(netrc_pw_mgr)
- ftp_handler = _NetrcFTPOpener(netrc_config)
- DownloadableFileSource.__urlopener = urllib.request.build_opener(http_auth, ftp_handler)
- return DownloadableFileSource.__urlopener
+
+class _UrlOpenerCreator:
+ def __init__(self):
+ self.netrc_config = None
+
+ try:
+ self.netrc_config = netrc.netrc()
+ except OSError:
+ # If the .netrc file was not found, FileNotFoundError will be
+ # raised, but OSError will be raised directly by the netrc package
+ # in the case that $HOME is not set.
+ #
+ # This will catch both cases.
+ pass
+ except netrc.NetrcParseError:
+ # netrc file exists but is invalid, passthrough since we can't log here
+ pass
+
+ def get_url_opener(self):
+ if self.netrc_config:
+ netrc_pw_mgr = _NetrcPasswordManager(self.netrc_config)
+ http_auth = urllib.request.HTTPBasicAuthHandler(netrc_pw_mgr)
+ ftp_handler = _NetrcFTPOpener(self.netrc_config)
+ return urllib.request.build_opener(http_auth, ftp_handler)
+ else:
+ return urllib.request.build_opener()
Review Comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on a diff in pull request #1820: Stop passing URL opener across process boundaries
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on code in PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#discussion_r1103643600
##########
src/buildstream/downloadablefilesource.py:
##########
@@ -260,29 +260,29 @@ def _get_mirror_file(self, sha=None):
return self.__default_mirror_file
- @classmethod
- def _reset_url_opener(cls):
- # Needed for tests, in order to cleanup the `netrc` configuration.
- cls.__urlopener = None # pylint: disable=unused-private-member
-
- def __get_urlopener(self):
- if not DownloadableFileSource.__urlopener:
- try:
- netrc_config = netrc.netrc()
- except OSError:
- # If the .netrc file was not found, FileNotFoundError will be
- # raised, but OSError will be raised directly by the netrc package
- # in the case that $HOME is not set.
- #
- # This will catch both cases.
- #
- DownloadableFileSource.__urlopener = urllib.request.build_opener()
- except netrc.NetrcParseError as e:
- self.warn("{}: While reading .netrc: {}".format(self, e))
- return urllib.request.build_opener()
- else:
- netrc_pw_mgr = _NetrcPasswordManager(netrc_config)
- http_auth = urllib.request.HTTPBasicAuthHandler(netrc_pw_mgr)
- ftp_handler = _NetrcFTPOpener(netrc_config)
- DownloadableFileSource.__urlopener = urllib.request.build_opener(http_auth, ftp_handler)
- return DownloadableFileSource.__urlopener
+
+class _UrlOpenerCreator:
+ def __init__(self):
+ self.netrc_config = None
+
+ try:
+ self.netrc_config = netrc.netrc()
+ except OSError:
+ # If the .netrc file was not found, FileNotFoundError will be
+ # raised, but OSError will be raised directly by the netrc package
+ # in the case that $HOME is not set.
+ #
+ # This will catch both cases.
+ pass
+ except netrc.NetrcParseError:
+ # netrc file exists but is invalid, passthrough since we can't log here
Review Comment:
I decoupled netrc parsing into a method in DownloadableFileSource since it was anyway parsed in main process so now it has access to logging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on pull request #1820: Stop passing URL opener across process boundaries
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on PR #1820:
URL: https://github.com/apache/buildstream/pull/1820#issuecomment-1407727065
Okay, done. I think there is some problem with buildgrid that is causing test failures.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org