fix: py_proto_library: append to PYTHONPATH less - #1553

Merged
aignas merged 1 commit into
bazel-contrib:mainfrom
tpudlik:proto_root_fix
Nov 14, 2023
Merged

fix: py_proto_library: append to PYTHONPATH less#1553
aignas merged 1 commit into
bazel-contrib:mainfrom
tpudlik:proto_root_fix

Conversation

@tpudlik

@tpudliktpudlik commented Nov 10, 2023•
edited
Loading

Copy link
Copy Markdown
Collaborator

Only append the [proto_root] to PYTHONPATH when it's actually necessary, i.e. when the generated Python modules are under _virtual_imports. Do no append it in the general case, when [proto_root] is just the repository root.

This makes it much less likely that adding a transitive dep on a py_proto_library will reorder the PYTHONPATH. Such reordering is undesirable because it may lead to import errors.

Fixes#1551

@tpudlik
fix: py_proto_library: append to PYTHONPATH less
2b05d9f
Only append the `[proto_root]` to PYTHONPATH when it's actually
necessary, i.e. when the generated Python modules are under
_virtual_imports. Do no append it in the general case, when
`[proto_root]` is just the repository root.
Fixesbazel-contrib#1551
aignas
aignas reviewed Nov 13, 2023
View reviewed changes

@aignasaignas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would wait for @rickeylev and/or @linzhp to also chime in.

Could you also please add a CHANGELOG.md entry and make the PR description be more like a commit message (which it will become)?

@tpudlik

tpudlik commented Nov 13, 2023

Copy link
Copy Markdown
CollaboratorAuthor

I updated the PR description.

Re CHANGELOG.md: the bug this fixes was never released, so is a changelog entry still desirable?

@aignas
aignas added this pull request to the merge queueNov 14, 2023
@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to failed status checks Nov 14, 2023
@aignas
aignas added this pull request to the merge queueNov 14, 2023
Merged via the queue into bazel-contrib:main with commit 85e50d2Nov 14, 2023
@aignasaignas mentioned this pull request Nov 14, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aignasaignasaignas approved these changes
@rickeylevrickeylevAwaiting requested review from rickeylevrickeylev is a code owner
+1 more reviewer
@linzhplinzhplinzhp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

py_proto_library prepends too aggressively to the PYTHONPATH

3 participants

@tpudlik@linzhp@aignas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
fix: py_proto_library: append to PYTHONPATH less - #1553
Merged
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Nov 14, 2023
Merged
fix: py_proto_library: append to PYTHONPATH less#1553
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Conversation
@tpudlik
@tpudliktpudlik
commented
Nov 10, 2023•

edited

Loading

Copy link


Copy Markdown

Collaborator
Only append the [proto_root] to PYTHONPATH when it's actually necessary, i.e. when the generated Python modules are under _virtual_imports. Do no append it in the general case, when [proto_root] is just the repository root.
This makes it much less likely that adding a transitive dep on a py_proto_library will reorder the PYTHONPATH. Such reordering is undesirable because it may lead to import errors.
Fixes#1551
@tpudlik
fix: py_proto_library: append to PYTHONPATH less
2b05d9f
Only append the `[proto_root]` to PYTHONPATH when it's actually
necessary, i.e. when the generated Python modules are under
_virtual_imports. Do no append it in the general case, when
`[proto_root]` is just the repository root.
Fixesbazel-contrib#1551
aignas
aignas
reviewed
Nov 13, 2023
View reviewed changes
@aignasaignas
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would wait for @rickeylev and/or @linzhp to also chime in.
Could you also please add a CHANGELOG.md entry and make the PR description be more like a commit message (which it will become)?
@tpudlik
tpudlik
commented
Nov 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
I updated the PR description.
Re CHANGELOG.md: the bug this fixes was never released, so is a changelog entry still desirable?
@aignas
aignas
added this pull request to the merge queueNov 14, 2023
@github-merge-queue
github-merge-queueBot
removed this pull request from the merge queue due to failed status checks
Nov 14, 2023
@aignas
aignas
added this pull request to the merge queueNov 14, 2023

Merged
via the queue into
bazel-contrib:main
with commit 85e50d2Nov 14, 2023
@aignasaignas
mentioned this pull request
Nov 14, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@aignasaignasaignas approved these changes
@rickeylevrickeylevAwaiting requested review from rickeylevrickeylev is a code owner

+1 more reviewer

@linzhplinzhplinzhp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

py_proto_library prepends too aggressively to the PYTHONPATH 

3 participants

@tpudlik@linzhp@aignas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix: py_proto_library: append to PYTHONPATH less - #1553
Merged
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Nov 14, 2023
Merged
fix: py_proto_library: append to PYTHONPATH less#1553
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Conversation
@tpudlik
@tpudliktpudlik
commented
Nov 10, 2023•

edited

Loading

Copy link


Copy Markdown

Collaborator
Only append the [proto_root] to PYTHONPATH when it's actually necessary, i.e. when the generated Python modules are under _virtual_imports. Do no append it in the general case, when [proto_root] is just the repository root.
This makes it much less likely that adding a transitive dep on a py_proto_library will reorder the PYTHONPATH. Such reordering is undesirable because it may lead to import errors.
Fixes#1551
@tpudlik
fix: py_proto_library: append to PYTHONPATH less
2b05d9f
Only append the `[proto_root]` to PYTHONPATH when it's actually
necessary, i.e. when the generated Python modules are under
_virtual_imports. Do no append it in the general case, when
`[proto_root]` is just the repository root.
Fixesbazel-contrib#1551
aignas
aignas
reviewed
Nov 13, 2023
View reviewed changes
@aignasaignas
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would wait for @rickeylev and/or @linzhp to also chime in.
Could you also please add a CHANGELOG.md entry and make the PR description be more like a commit message (which it will become)?
@tpudlik
tpudlik
commented
Nov 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
I updated the PR description.
Re CHANGELOG.md: the bug this fixes was never released, so is a changelog entry still desirable?
@aignas
aignas
added this pull request to the merge queueNov 14, 2023
@github-merge-queue
github-merge-queueBot
removed this pull request from the merge queue due to failed status checks
Nov 14, 2023
@aignas
aignas
added this pull request to the merge queueNov 14, 2023

Merged
via the queue into
bazel-contrib:main
with commit 85e50d2Nov 14, 2023
@aignasaignas
mentioned this pull request
Nov 14, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@aignasaignasaignas approved these changes
@rickeylevrickeylevAwaiting requested review from rickeylevrickeylev is a code owner

+1 more reviewer

@linzhplinzhplinzhp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

py_proto_library prepends too aggressively to the PYTHONPATH 

3 participants

@tpudlik@linzhp@aignas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix: py_proto_library: append to PYTHONPATH less - #1553
Merged
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Nov 14, 2023
Merged
fix: py_proto_library: append to PYTHONPATH less#1553
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Conversation
@tpudlik
@tpudliktpudlik
commented
Nov 10, 2023•

edited

Loading

Copy link


Copy Markdown

Collaborator
Only append the [proto_root] to PYTHONPATH when it's actually necessary, i.e. when the generated Python modules are under _virtual_imports. Do no append it in the general case, when [proto_root] is just the repository root.
This makes it much less likely that adding a transitive dep on a py_proto_library will reorder the PYTHONPATH. Such reordering is undesirable because it may lead to import errors.
Fixes#1551
@tpudlik
fix: py_proto_library: append to PYTHONPATH less
2b05d9f
Only append the `[proto_root]` to PYTHONPATH when it's actually
necessary, i.e. when the generated Python modules are under
_virtual_imports. Do no append it in the general case, when
`[proto_root]` is just the repository root.
Fixesbazel-contrib#1551
aignas
aignas
reviewed
Nov 13, 2023
View reviewed changes
@aignasaignas
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would wait for @rickeylev and/or @linzhp to also chime in.
Could you also please add a CHANGELOG.md entry and make the PR description be more like a commit message (which it will become)?
@tpudlik
tpudlik
commented
Nov 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
I updated the PR description.
Re CHANGELOG.md: the bug this fixes was never released, so is a changelog entry still desirable?
@aignas
aignas
added this pull request to the merge queueNov 14, 2023
@github-merge-queue
github-merge-queueBot
removed this pull request from the merge queue due to failed status checks
Nov 14, 2023
@aignas
aignas
added this pull request to the merge queueNov 14, 2023

Merged
via the queue into
bazel-contrib:main
with commit 85e50d2Nov 14, 2023
@aignasaignas
mentioned this pull request
Nov 14, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@aignasaignasaignas approved these changes
@rickeylevrickeylevAwaiting requested review from rickeylevrickeylev is a code owner

+1 more reviewer

@linzhplinzhplinzhp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

py_proto_library prepends too aggressively to the PYTHONPATH 

3 participants

@tpudlik@linzhp@aignas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
fix: py_proto_library: append to PYTHONPATH less - #1553
Merged
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Nov 14, 2023
Merged
fix: py_proto_library: append to PYTHONPATH less#1553
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Conversation
@tpudlik
@tpudliktpudlik
commented
Nov 10, 2023•

edited

Loading

Copy link


Copy Markdown

Collaborator
Only append the [proto_root] to PYTHONPATH when it's actually necessary, i.e. when the generated Python modules are under _virtual_imports. Do no append it in the general case, when [proto_root] is just the repository root.
This makes it much less likely that adding a transitive dep on a py_proto_library will reorder the PYTHONPATH. Such reordering is undesirable because it may lead to import errors.
Fixes#1551
@tpudlik
fix: py_proto_library: append to PYTHONPATH less
2b05d9f
Only append the `[proto_root]` to PYTHONPATH when it's actually
necessary, i.e. when the generated Python modules are under
_virtual_imports. Do no append it in the general case, when
`[proto_root]` is just the repository root.
Fixesbazel-contrib#1551
aignas
aignas
reviewed
Nov 13, 2023
View reviewed changes
@aignasaignas
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would wait for @rickeylev and/or @linzhp to also chime in.
Could you also please add a CHANGELOG.md entry and make the PR description be more like a commit message (which it will become)?
@tpudlik
tpudlik
commented
Nov 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
I updated the PR description.
Re CHANGELOG.md: the bug this fixes was never released, so is a changelog entry still desirable?
@aignas
aignas
added this pull request to the merge queueNov 14, 2023
@github-merge-queue
github-merge-queueBot
removed this pull request from the merge queue due to failed status checks
Nov 14, 2023
@aignas
aignas
added this pull request to the merge queueNov 14, 2023

Merged
via the queue into
bazel-contrib:main
with commit 85e50d2Nov 14, 2023
@aignasaignas
mentioned this pull request
Nov 14, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@aignasaignasaignas approved these changes
@rickeylevrickeylevAwaiting requested review from rickeylevrickeylev is a code owner

+1 more reviewer

@linzhplinzhplinzhp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

py_proto_library prepends too aggressively to the PYTHONPATH 

3 participants

@tpudlik@linzhp@aignas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix: py_proto_library: append to PYTHONPATH less - #1553
Merged
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Nov 14, 2023
Merged
fix: py_proto_library: append to PYTHONPATH less#1553
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Conversation
@tpudlik
@tpudliktpudlik
commented
Nov 10, 2023•

edited

Loading

Copy link


Copy Markdown

Collaborator
Only append the [proto_root] to PYTHONPATH when it's actually necessary, i.e. when the generated Python modules are under _virtual_imports. Do no append it in the general case, when [proto_root] is just the repository root.
This makes it much less likely that adding a transitive dep on a py_proto_library will reorder the PYTHONPATH. Such reordering is undesirable because it may lead to import errors.
Fixes#1551
@tpudlik
fix: py_proto_library: append to PYTHONPATH less
2b05d9f
Only append the `[proto_root]` to PYTHONPATH when it's actually
necessary, i.e. when the generated Python modules are under
_virtual_imports. Do no append it in the general case, when
`[proto_root]` is just the repository root.
Fixesbazel-contrib#1551
aignas
aignas
reviewed
Nov 13, 2023
View reviewed changes
@aignasaignas
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would wait for @rickeylev and/or @linzhp to also chime in.
Could you also please add a CHANGELOG.md entry and make the PR description be more like a commit message (which it will become)?
@tpudlik
tpudlik
commented
Nov 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
I updated the PR description.
Re CHANGELOG.md: the bug this fixes was never released, so is a changelog entry still desirable?
@aignas
aignas
added this pull request to the merge queueNov 14, 2023
@github-merge-queue
github-merge-queueBot
removed this pull request from the merge queue due to failed status checks
Nov 14, 2023
@aignas
aignas
added this pull request to the merge queueNov 14, 2023

Merged
via the queue into
bazel-contrib:main
with commit 85e50d2Nov 14, 2023
@aignasaignas
mentioned this pull request
Nov 14, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@aignasaignasaignas approved these changes
@rickeylevrickeylevAwaiting requested review from rickeylevrickeylev is a code owner

+1 more reviewer

@linzhplinzhplinzhp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

py_proto_library prepends too aggressively to the PYTHONPATH 

3 participants

@tpudlik@linzhp@aignas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix: py_proto_library: append to PYTHONPATH less - #1553
Merged
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Nov 14, 2023
Merged
fix: py_proto_library: append to PYTHONPATH less#1553
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Conversation
@tpudlik
@tpudliktpudlik
commented
Nov 10, 2023•

edited

Loading

Copy link


Copy Markdown

Collaborator
Only append the [proto_root] to PYTHONPATH when it's actually necessary, i.e. when the generated Python modules are under _virtual_imports. Do no append it in the general case, when [proto_root] is just the repository root.
This makes it much less likely that adding a transitive dep on a py_proto_library will reorder the PYTHONPATH. Such reordering is undesirable because it may lead to import errors.
Fixes#1551
@tpudlik
fix: py_proto_library: append to PYTHONPATH less
2b05d9f
Only append the `[proto_root]` to PYTHONPATH when it's actually
necessary, i.e. when the generated Python modules are under
_virtual_imports. Do no append it in the general case, when
`[proto_root]` is just the repository root.
Fixesbazel-contrib#1551
aignas
aignas
reviewed
Nov 13, 2023
View reviewed changes
@aignasaignas
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would wait for @rickeylev and/or @linzhp to also chime in.
Could you also please add a CHANGELOG.md entry and make the PR description be more like a commit message (which it will become)?
@tpudlik
tpudlik
commented
Nov 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
I updated the PR description.
Re CHANGELOG.md: the bug this fixes was never released, so is a changelog entry still desirable?
@aignas
aignas
added this pull request to the merge queueNov 14, 2023
@github-merge-queue
github-merge-queueBot
removed this pull request from the merge queue due to failed status checks
Nov 14, 2023
@aignas
aignas
added this pull request to the merge queueNov 14, 2023

Merged
via the queue into
bazel-contrib:main
with commit 85e50d2Nov 14, 2023
@aignasaignas
mentioned this pull request
Nov 14, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@aignasaignasaignas approved these changes
@rickeylevrickeylevAwaiting requested review from rickeylevrickeylev is a code owner

+1 more reviewer

@linzhplinzhplinzhp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

py_proto_library prepends too aggressively to the PYTHONPATH 

3 participants

@tpudlik@linzhp@aignas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
fix: py_proto_library: append to PYTHONPATH less - #1553
Merged
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Nov 14, 2023
Merged
fix: py_proto_library: append to PYTHONPATH less#1553
aignas merged 1 commit into
bazel-contrib:mainfrom 
tpudlik:proto_root_fix
Conversation
@tpudlik
@tpudliktpudlik
commented
Nov 10, 2023•

edited

Loading

Copy link


Copy Markdown

Collaborator
Only append the [proto_root] to PYTHONPATH when it's actually necessary, i.e. when the generated Python modules are under _virtual_imports. Do no append it in the general case, when [proto_root] is just the repository root.
This makes it much less likely that adding a transitive dep on a py_proto_library will reorder the PYTHONPATH. Such reordering is undesirable because it may lead to import errors.
Fixes#1551
@tpudlik
fix: py_proto_library: append to PYTHONPATH less
2b05d9f
Only append the `[proto_root]` to PYTHONPATH when it's actually
necessary, i.e. when the generated Python modules are under
_virtual_imports. Do no append it in the general case, when
`[proto_root]` is just the repository root.
Fixesbazel-contrib#1551
aignas
aignas
reviewed
Nov 13, 2023
View reviewed changes
@aignasaignas
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would wait for @rickeylev and/or @linzhp to also chime in.
Could you also please add a CHANGELOG.md entry and make the PR description be more like a commit message (which it will become)?
@tpudlik
tpudlik
commented
Nov 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
I updated the PR description.
Re CHANGELOG.md: the bug this fixes was never released, so is a changelog entry still desirable?
@aignas
aignas
added this pull request to the merge queueNov 14, 2023
@github-merge-queue
github-merge-queueBot
removed this pull request from the merge queue due to failed status checks
Nov 14, 2023
@aignas
aignas
added this pull request to the merge queueNov 14, 2023

Merged
via the queue into
bazel-contrib:main
with commit 85e50d2Nov 14, 2023
@aignasaignas
mentioned this pull request
Nov 14, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@aignasaignasaignas approved these changes
@rickeylevrickeylevAwaiting requested review from rickeylevrickeylev is a code owner

+1 more reviewer

@linzhplinzhplinzhp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

py_proto_library prepends too aggressively to the PYTHONPATH 

3 participants

@tpudlik@linzhp@aignas