text:
When initializing a packed scene, if the packed scene root has a script that chooses a parent on _init(), causes errors and will cause segfault.

Tested versions
4.3.1.rc
System information
Ubuntu 22.04.4 LTS 64-bit
Issue description
I have a class_name Foo that chooses its parent via a method of a Autoload/Singleton. Foo.new() works fine. However, if I have a load a packed scene whose root is Foo (load("foo.tscn")), trying to remove the child from the parent creates a "Children name does not match parent name in hashtable" error.
Closing the window via the [x] button / quit via the system context menu causes a segfault.
Freeing the object will silently crash.
ERROR: Children name does not match parent name in hashtable, this is a bug.
   at: remove_child (scene/main/node.cpp:1625)
Orphans:<Node3D#31423727074>
ERROR: Parameter "get_viewport()" is null.
   at: _notification (scene/main/node.cpp:144)
ERROR: Children name does not match parent name in hashtable, this is a bug.
   at: remove_child (scene/main/node.cpp:1625)
ERROR: Condition "data.parent" is true.
   at: ~Node (scene/main/node.cpp:3847)

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.3.1.rc.custom_build (ff9bc0422349219b337b015643544a0454d4a7ee)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x7c8191a42520] (??:0)
[2] Object::notification(int, bool) (/home/leonard/Git/godot/core/object/object.cpp:884)
[3] Object::_predelete() (/home/leonard/Git/godot/core/object/object.cpp:199)
[4] predelete_handler(Object*) (/home/leonard/Git/godot/core/object/object.cpp:2125)
[5] void memdelete<Node>(Node*) (/home/leonard/Git/godot/./core/os/memory.h:112)
[6] Node::_notification(int) (/home/leonard/Git/godot/scene/main/node.cpp:245)
[7] Node::_notificationv(int, bool) (/home/leonard/Git/godot/./scene/main/node.h:50 (discriminator 14))
[8] Node3D::_notificationv(int, bool) (/home/leonard/Git/godot/./scene/3d/node_3d.h:52)
[9] Object::notification(int, bool) (/home/leonard/Git/godot/core/object/object.cpp:890)
[10] Object::_predelete() (/home/leonard/Git/godot/core/object/object.cpp:199)
[11] predelete_handler(Object*) (/home/leonard/Git/godot/core/object/object.cpp:2125)
[12] void memdelete<Node>(Node*) (/home/leonard/Git/godot/./core/os/memory.h:112)
[13] Node::_notification(int) (/home/leonard/Git/godot/scene/main/node.cpp:245)
[14] Node::_notificationv(int, bool) (/home/leonard/Git/godot/./scene/main/node.h:50 (discriminator 14))
[15] CanvasItem::_notificationv(int, bool) (/home/leonard/Git/godot/./scene/main/canvas_item.h:45)
[16] Control::_notificationv(int, bool) (/home/leonard/Git/godot/./scene/gui/control.h:48)
[17] Object::notification(int, bool) (/home/leonard/Git/godot/core/object/object.cpp:890)
[18] Object::_predelete() (/home/leonard/Git/godot/core/object/object.cpp:199)
[19] predelete_handler(Object*) (/home/leonard/Git/godot/core/object/object.cpp:2125)
[20] void memdelete<Node>(Node*) (/home/leonard/Git/godot/./core/os/memory.h:112)
[21] Node::_notification(int) (/home/leonard/Git/godot/scene/main/node.cpp:245)
[22] Node::_notificationv(int, bool) (/home/leonard/Git/godot/./scene/main/node.h:50 (discriminator 14))
[23] Viewport::_notificationv(int, bool) (/home/leonard/Git/godot/./scene/main/viewport.h:95)
[24] Window::_notificationv(int, bool) (/home/leonard/Git/godot/./scene/main/window.h:44)
[25] Object::notification(int, bool) (/home/leonard/Git/godot/core/object/object.cpp:890)
[26] Object::_predelete() (/home/leonard/Git/godot/core/object/object.cpp:199)
[27] predelete_handler(Object*) (/home/leonard/Git/godot/core/object/object.cpp:2125)
[28] void memdelete<Window>(Window*) (/home/leonard/Git/godot/./core/os/memory.h:112)
[29] SceneTree::finalize() (/home/leonard/Git/godot/scene/main/scene_tree.cpp:645)
[30] OS_LinuxBSD::run() (/home/leonard/Git/godot/platform/linuxbsd/os_linuxbsd.cpp:967)
[31] /home/leonard/Git/godot/bin/godot.linuxbsd.editor.dev.x86_64(main+0x190) [0x5f09e01af539] (/home/leonard/Git/godot/platform/linuxbsd/godot_linuxbsd.cpp:85)
[32] /lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x7c8191a29d90] (??:0)
[33] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) [0x7c8191a29e40] (??:0)
[34] /home/leonard/Git/godot/bin/godot.linuxbsd.editor.dev.x86_64(_start+0x25) [0x5f09e01af2e5] (??:?)
-- END OF BACKTRACE --
================================================================

Steps to reproduce
I have attached an MRP.
The class_name Foo accesses a singleton to be able to be added to the tree.
The main script will load a packedscene of Foo on ready, which is when the issues should start.
Minimal reproduction project (MRP)
parent_crash.zip

Bisecting points to #75627 as the culprit, @reduz

Errors + backtrace in vscode
ERROR: Children name does not match parent name in hashtable, this is a bug.
   at: (scene\main\node.cpp:1199)
Children name does not match parent name in hashtable, this is a bug.
Orphans:<Node3D#25937577089>
ERROR: Condition "!get_viewport()" is true.
   at: Node::_notification (scene\main\node.cpp:98)
scene\main\node.cpp:98 - Condition "!get_viewport()" is true.
ERROR: Children name does not match parent name in hashtable, this is a bug.
   at: (scene\main\node.cpp:1199)
Children name does not match parent name in hashtable, this is a bug.
ERROR: Condition "data.parent" is true.
   at: Node::~Node (scene\main\node.cpp:3086)
scene\main\node.cpp:3086 - Condition "data.parent" is true.
Critical error detected c0000374

ntdll.dll!00007ffbe08af3c2() (Unknown Source:0)
ntdll.dll!00007ffbe08b8182() (Unknown Source:0)
ntdll.dll!00007ffbe08b846a() (Unknown Source:0)
ntdll.dll!00007ffbe08be0f1() (Unknown Source:0)
ntdll.dll!00007ffbe07d5bf0() (Unknown Source:0)
ntdll.dll!00007ffbe07d47b1() (Unknown Source:0)
AcLayers.dll!00007ffb73278931() (Unknown Source:0)
_free_base(void * block) Line 105 (c:\Users\Matheus\AppData\Local\Programs\Microsoft VS Code\minkernel\crts\ucrt\src\appcrt\heap\free_base.cpp:105)
Memory::free_static(void * p_ptr, bool p_pad_align) Line 169 (c:\Users\Matheus\Downloads\Godot Source\core\os\memory.cpp:169)
memdelete<Node>(Node * p_class) Line 113 (c:\Users\Matheus\Downloads\Godot Source\core\os\memory.h:113)
Node::_notification(int p_notification) Line 171 (c:\Users\Matheus\Downloads\Godot Source\scene\main\node.cpp:171)
Node::_notificationv(int p_notification, bool p_reversed) Line 46 (c:\Users\Matheus\Downloads\Godot Source\scene\main\node.h:46)
Node3D::_notificationv(int p_notification, bool p_reversed) Line 52 (c:\Users\Matheus\Downloads\Godot Source\scene\3d\node_3d.h:52)
Object::notification(int p_notification, bool p_reversed) Line 792 (c:\Users\Matheus\Downloads\Godot Source\core\object\object.cpp:792)
Object::_predelete() Line 197 (c:\Users\Matheus\Downloads\Godot Source\core\object\object.cpp:197)
predelete_handler(Object * p_object) Line 1836 (c:\Users\Matheus\Downloads\Godot Source\core\object\object.cpp:1836)
memdelete<Node>(Node * p_class) Line 105 (c:\Users\Matheus\Downloads\Godot Source\core\os\memory.h:105)
Node::_notification(int p_notification) Line 171 (c:\Users\Matheus\Downloads\Godot Source\scene\main\node.cpp:171)
Node::_notificationv(int p_notification, bool p_reversed) Line 46 (c:\Users\Matheus\Downloads\Godot Source\scene\main\node.h:46)
CanvasItem::_notificationv(int p_notification, bool p_reversed) Line 45 (c:\Users\Matheus\Downloads\Godot Source\scene\main\canvas_item.h:45)
Control::_notificationv(int p_notification, bool p_reversed) Line 47 (c:\Users\Matheus\Downloads\Godot Source\scene\gui\control.h:47)
Object::notification(int p_notification, bool p_reversed) Line 792 (c:\Users\Matheus\Downloads\Godot Source\core\object\object.cpp:792)
Object::_predelete() Line 197 (c:\Users\Matheus\Downloads\Godot Source\core\object\object.cpp:197)
predelete_handler(Object * p_object) Line 1836 (c:\Users\Matheus\Downloads\Godot Source\core\object\object.cpp:1836)
memdelete<Node>(Node * p_class) Line 105 (c:\Users\Matheus\Downloads\Godot Source\core\os\memory.h:105)
Node::_notification(int p_notification) Line 171 (c:\Users\Matheus\Downloads\Godot Source\scene\main\node.cpp:171)
Node::_notificationv(int p_notification, bool p_reversed) Line 46 (c:\Users\Matheus\Downloads\Godot Source\scene\main\node.h:46)
Viewport::_notificationv(int p_notification, bool p_reversed) Line 89 (c:\Users\Matheus\Downloads\Godot Source\scene\main\viewport.h:89)
Window::_notificationv(int p_notification, bool p_reversed) Line 44 (c:\Users\Matheus\Downloads\Godot Source\scene\main\window.h:44)
Object::notification(int p_notification, bool p_reversed) Line 792 (c:\Users\Matheus\Downloads\Godot Source\core\object\object.cpp:792)
Object::_predelete() Line 197 (c:\Users\Matheus\Downloads\Godot Source\core\object\object.cpp:197)
predelete_handler(Object * p_object) Line 1836 (c:\Users\Matheus\Downloads\Godot Source\core\object\object.cpp:1836)
memdelete<Window>(Window * p_class) Line 105 (c:\Users\Matheus\Downloads\Godot Source\core\os\memory.h:105)
SceneTree::finalize() Line 585 (c:\Users\Matheus\Downloads\Godot Source\scene\main\scene_tree.cpp:585)
OS_Windows::run() Line 1306 (c:\Users\Matheus\Downloads\Godot Source\platform\windows\os_windows.cpp:1306)
widechar_main(int argc, wchar_t * * argv) Line 181 (c:\Users\Matheus\Downloads\Godot Source\platform\windows\godot_windows.cpp:181)
_main() Line 203 (c:\Users\Matheus\Downloads\Godot Source\platform\windows\godot_windows.cpp:203)
main(int argc, char * * argv) Line 217 (c:\Users\Matheus\Downloads\Godot Source\platform\windows\godot_windows.cpp:217)
WinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, char * lpCmdLine, int nCmdShow) Line 231 (c:\Users\Matheus\Downloads\Godot Source\platform\windows\godot_windows.cpp:231)
[Inline Frame] invoke_main() Line 102 (d:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:102)
__scrt_common_main_seh() Line 288 (d:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
kernel32.dll!00007ffbdff37374() (Unknown Source:0)
ntdll.dll!00007ffbe07fcc91() (Unknown Source:0)

Went ahead and tested the results when there was a failure to find the child. It appears that it is saving the node with an autogenerated name rather than Foo on the Orphan node.

First value is the StringName of the hashmap data.children, second value is the name of the Node pointer in the hashmap data.children and the third value is the name of the node passed into the remove_child function.
Creating the node using new on the packed scene causes the scene to be named @Node@2 and save itself as a Node type rather than the Node3d type as in the scene itself.
From what I have tested, inside of _init the scene is yet to have a name, but using instantiate will eventually set the name to the root node name, while new does not. Setting the name to Foo inside of _init will make the instantiate method of constructing work without crashing, as well as working with the new method of constructing the packed scene.
Not as familiar with the API yet, so unsure of the best solution, but it would seem that having the packed scene set its name early on or blocking other threads of execution until the name is set would be the "proper" fix.

authors: AtlaStar OffsetMOSFET matheusmdx


text:
queue_redraw triggers redraw immediately multiple times per frame without queuing

Tested versions
Tested versions

Reproducible in 4.3.stable
Not reproducible in 4.2.2.stable

System information
Godot v4.3.stable - macOS 14.6.1 - Vulkan (Forward+) - integrated Apple M1 - Apple M1 (8 Threads)
Issue description
Calling queue_redraw() somewhere in the callstack of a node script's _draw() function caused it to redraw continuously, hanging the game on frame 0.
Although queue_redraw()'s documentation says a redraw is triggered only once per frame even when you call it multiple times, that is evidently not the case in 4.3.stable.
When inserting a workaround provided in another issue where there was a queuing regression https://github.com/godotengine/godot/issues/88953#issuecomment-1968907901, the redraw is forced to wait a frame and the problem went away.
Steps to reproduce
Run MRP project. Test in both 4.2.2.stable and 4.3.stable to see the difference. Uncomment the workaround provided in the project.

In 4.2.2.stable, the TextureRect starts with a green border. When you hover over it, it has a red border. When mouse_exit is triggered, you have a blue border.
Note that in 4.3.stable, the print statement is printing the current frame 0 thousands of times. You probably will crash and not load properly.
In the provided commented out workaround, redraw is only called after the current frame is processed, and so the game loads and runs normally.

Minimal reproduction project (MRP)
queue_redraw-triggers-redraw-immediately-without-waiting-1-frame.zip

This sounds like:

https://github.com/godotengine/godot/issues/95709
https://github.com/godotengine/godot/issues/95810

And is solved by:

https://github.com/godotengine/godot/pull/97328

If you are able to test please confirm, but should be that bug

source: gharchive/issue

authors: AThousandShips anthonyronda

repo: godotengine/godot

license: MIT

license_type: permissive

license_source: github-api


text:
Fix Python 3 incompatibility in Mono build

This fixes the Python 3 incompatibility when trying to build the Mono module mentioned in #11860.
Building the engine with module_mono_enabled=yes fails for me during the link stage because of missing symbols GodotSharpBindings::get_core_api_hash, GodotSharpBindings::get_editor_api_hash(void) and GodotSharpBindings::register_generated_icalls(void) but this seems to be unrelated to the Python 3 issue.

that was quick :)

I aim to please ;)

source: gharchive/pull-request

authors: hoelzl novemberist


text:
Add method to clear modal stack in the Viewport

This function closes any modal (popups, dialogs, etc) that are present in the viewport.
This is very useful if you have a menu that can toggled with a key press, removing the necessity to keep track of all modals in the GUI to close them.

I can understand the need for this function, but you can do this easily with:
while (get_viewport().get_modal_stack_top()):
    get_viewport().get_modal_stack_top().hide()

so i dont think this is worth it.

@reduz There's no get_modal_stack_top() in GDScript.

It could likely be bound.

@akien-mga OK, I got the hint. Going to make a new pull exposing that, closing.

authors: YeldhamDev akien-mga reduz


text:
Prevent FileDialog from stealing focus when setting current file in editor

Should close #23619
The issue also mentions that you can input "/" characters for the current file property. I suppose it isn't a problem since if the filename is invalid it will throw an error while opening/saving it, but should something be done about that ?

Thanks!

authors: Kanabenki akien-mga


text:
Move PopupWindow logic to GodotEditText on Android

Same as #40791 for master.
Just some cleaning from #40672: moving the PopupWindow logic to GodotEditText and everything in GodotEditText in now setup in the constructor.

Thanks!

authors: akien-mga pouleyKetchoupp


text:
Make intersect_ray() return true, if ray orginates inside of a shape.

Fixes #35845
Fixes #38343
Fixes #38873
Fixes #39859
Fixes #44003
Supersedes #41277
Addresses comment in #41167.
The issues above suggest that, if the ray originates inside of a shape, intersect_ray() is expected to return true. This change performs this check and assigns the first shape that it detects that the ray is within as the colliding Object. It also sets the colliding point to the ray origin. Since the ray is not colliding with an edge or face, there is no normal; so the zero length vector is assigned. This makes all CollisionShapes consistent with the current results when a ray originates inside a Rectangle2D or BoxShape3D (in Godot physics).
This change has been applied to both Godot 2D and Godot 3D physics. It has not been applied to Bullet physics, which uses the opposite approach: returns no collision if the ray originates inside a shape (has an incalculable normal); as proposed in #41277. However, unlike in Godot physics, in Bullet physics, there does not appear to be an easy way to determine if a point (the ray origin) is contained inside a CollisionShape to make this consistent in Bullet physics too.

This will be good, so when it intersects at the start point it will always be a zero normal, and it can be consistently detected.
It would be nice to amend the documentation about that part.  I find it's not intuitive to expect a field called normal to return a 0 length.  I would probably pickup on the possibility of this behavior if there were a boolean field about this kind of intersection in the result dictionary.
I was once wondering about this situation.  Though it requires more working around, I lean towards preferring the behavior in this PR so there is information about the starting point intersection occurring.  That aside, I was wondering what your thoughts about these two ideas would be.  (Both would probably need an additional field to the dictionary indicating that the start point intersected.)
A) The ray's starting point is extended backwards until it is not intersecting, and use that as the start point.

B) Find the nearest outside position, and use that as the point of collision.

The fact that there are at least two options proves that there isn't a correct solution. Personally, I think the correct solution is the Bullet physics approach i.e. not return a collision if the ray originates inside of a shape. However, with this PR, in Godot physics, this can be achieved by simply also checking if the normal is a zero vector:
if get_collider() and get_collision_normal() != Vector2():
	#Not colliding and not originating inside of a collider
	...

Using a zero vector enables this.

Updated to include updates to the relevant class documentation.

I think the correct solution is the Bullet physics approach i.e. not return a collision if the ray originates inside of a shape.

Without having to a do point intersection test before the ray cast, how does one know this situation has occurred?
In more than one situation I've had a raycast coming from something moving and was using only raycasts (not safely from inside a collider), and could slip into geometry.  I needed to catch this situation, because otherwise the algorithm I was using would get trapped, as it relied on some type of outward normal to repel away.  If there was no collision result at all, these things would've carried on ghosting through walls.
The point of confusion around this issue in these posts (including mine), is from unexpected results.  Until this PR the documentation is not making any mention of this failing condition.
This condition is such an edge case that it doesn't really appear right away.  Related to my post, it wasn't until several weeks after finishing the initial work did an object find itself caught in a sharp corner.  It just seemed really mysterious where the problem may be.  It was also dreadfully difficult to reproduce.
I typically comb over my algorithms first, assuming I missing something.
It took some additional time to create the experiments to tease out the behavior from the engine and work around it.
I don't think general users or beginners are going to ever get this far with it.  Given how many issues linked so far, this is a serious hole that you're patching up.  👍
As a post thought to the slipperiness of this:  It may be drastic, but better still to pass back a null normal instead of an empty vector, and perhaps even a null position.  This way trying to directly handle these fields as a vector will cause a crash and point immediately to the source.
var result = intersect_ray(...)
position = result.position # null

var result = intersect_ray(...)
position = result.normal * speed # null * speed

var result = intersect_ray(...)

# check result
if(not result.empty() and result.normal):

People asking for help on null normals or positions, or looking at the documentation should now get a swifter resolution.

Rebased following #41954.

I'm a bit concerned with this being inconsistent in 3D with GodotPhysics vs Bullet. At least it's well documented, but it would be best if they behaved similarly.
Otherwise this seems good as it fixes many valid issues. There's always the concern that some users might be relying on this behavior as a feature, but I guess they'll be able to get it back using collision layers?

Updated with the suggested changes to the documentation.

I'm a bit concerned with this being inconsistent in 3D with GodotPhysics vs Bullet. At least it's well documented, but it would be best if they behaved similarly.

Unfortunately, in Bullet physics, there does not appear to be an easy way to determine if a point (the ray origin) is contained inside a CollisionShape. To make them consistent I originally suggested #41277, but, as the issues this PR closes indicate, this is not the expected behaviour.

This code at the beginning of BulletPhysicsDirectSpaceState::intersect_ray will do the trick for Bullet:
ShapeResult result;
if (0 < intersect_point(p_from, &result, 1, p_exclude, p_collision_mask, p_collide_with_bodies, p_collide_with_areas)) {
	r_result.position = p_from;
	r_result.normal = Vector3();
	r_result.shape = result.shape;
	r_result.rid = result.rid;
	r_result.collider_id = result.collider_id;
	r_result.collider = 0 == r_result.collider_id ? NULL : ObjectDB::get_instance(r_result.collider_id);
	return true;
}

The changes in GodotPhysics look fine. I think it makes sense to have this behavior by default, and harmonize all the physics backends.
Lots of users will expect the behavior to be the other way around though since it's how it's been working. A very common use case is raycasts starting from inside a character controller, and if we don't communicate properly about this change of behavior, we might get a ton of extra issues.
@akien-mga: Do we have a specific place to list changes we need to communicate for different releases? Or is it based on breaks-compat tags? We need to make sure there will be a warning for users who port their projects to 4.0, as well as in the 3.2.4 release notes if this change is cherry-picked.

@akien-mga: Do we have a specific place to list changes we need to communicate for different releases? Or is it based on breaks-compat tags? We need to make sure there will be a warning for users who port their projects to 4.0, as well as in the 3.2.4 release notes if this change is cherry-picked.

The release post as well as the changelog are typically used to list compatibility breakage.

@madmiraal Could you add the Bullet fix from https://github.com/godotengine/godot/pull/41347#issuecomment-742747719 and confirm it works? It would be nice to include this PR in 3.2.4 once all the physics backends are consistent.

@madmiraal Could you add the Bullet fix from https://github.com/godotengine/godot/pull/41347#issuecomment-742747719 and confirm it works? It would be nice to include this PR in 3.2.4 once all the physics backends are consistent.

I don't think the suggestion is a good one. Bullet's implementation of intersect_point() is a hack to get the Bullet physics interface to work. It basically creates a small sphere and then checks for collisions. As I stated in the OP:

... in Bullet physics, there does not appear to be an easy way to determine if a point (the ray origin) is contained inside a CollisionShape ...

I think the difference should be documented, and, if the user wants this behaviour, they can perform the intersect_point() test themselves.

I don't think the suggestion is a good one. Bullet's implementation of intersect_point() is a hack to get the Bullet physics interface to work. It basically creates a small sphere and then checks for collisions. As I stated in the OP:

... in Bullet physics, there does not appear to be an easy way to determine if a point (the ray origin) is contained inside a CollisionShape ...

I think the difference should be documented, and, if the user wants this behaviour, they can perform the intersect_point() test themselves.

@madmiraal The technical difference between a very small sphere and a point is less important than making the physics backends consistent.
Given that Bullet is currently the default engine, you must include this fix for the PR to be merged into 3.2.

@madmiraal The technical difference between a very small sphere and a point is less important than making the physics backends consistent.
Given that Bullet is currently the default engine, you must include this fix for the PR to be merged into 3.2.

I really think this should be optional, there are too many cases where you want either one or the other behavior.

Superseded by #54857.

authors: Calinou akien-mga avencherus madmiraal pouleyKetchoupp reduz


text:
Expose soft body pin methods to GDScript

This change was merged into master #51364 but requires class name changes for 3.x in code and documentation.
Node3D changed to Spatial and SoftBody3D to SoftBody.

My bad, there's a legitimate error in mono:
/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/SoftBody.cs(634,74): error CS1750: A value of type 'string' cannot be used as a default parameter because there are no standard conversions to type 'NodePath' [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj]

I'm not sure why this is happening. Maybe something wrong with an empty NodePath as default parameter.

I've just tested locally, it looks like it could be a bug in mono (maybe fixed only on master since this error happens on 3.x only).
The generated mono glue for the new exposed method is:
[DebuggerBrowsable(DebuggerBrowsableState.Never)]
private static IntPtr method_bind_34 = Object.godot_icall_Object_ClassDB_get_method(nativeName, "set_point_pinned");

[GodotMethod("set_point_pinned")]
public void SetPointPinned(int pointIndex, bool pinned, NodePath attachmentPath = "")
{
    NativeCalls.godot_icall_3_802(method_bind_34, Object.GetPtr(this), pointIndex, pinned, NodePath.GetPtr(attachmentPath));
}

Although the node path argument should be NodePath attachmentPath = NodePath() or NodePath attachmentPath = NodePath("")
@neikeq Do you know what could cause this issue?

@pouleyKetchoupp
This:
https://github.com/godotengine/godot/blob/ef1c5093727d22d18d3f02eefb6f7e8e5e98eadd/modules/mono/editor/bindings_generator.cpp#L2578-L2581
should be:
if (r_iarg.type.cname == name_cache.type_NodePath) {
	r_iarg.default_argument = "(%s)\"" + r_iarg.default_argument + "\"";
	r_iarg.def_param_mode = ArgumentInterface::NULLABLE_REF;
} else {
	CRASH_COND(r_iarg.type.cname != name_cache.type_String);
	r_iarg.default_argument = "\"" + r_iarg.default_argument + "\"";
}
break;

to match current master.

Rebasing on latest 3.x should fix the Mono build issue.

Please squash your commits (see interactive rebase) and it should be good to go!

authors: akien-mga bengtsts neikeq pouleyKetchoupp


text:
Fix "Vector4DDecompose" typo in VisualShaderEditorPlugin

Fixes a typo introduced by #60175.

Thanks!

authors: Chaosus aaronfranke


text:
Add text overrun behavior for the theme type selector

This PR makes sure that even if a theme type has an unreasonably long name, it will no longer break the editor UI.

This uses the new text overrun behavior for buttons (https://github.com/godotengine/godot/pull/61819) similar to https://github.com/godotengine/godot/pull/61702. Unfortunately, it doesn't look as nice as it should, because there seems to be a regression in this feature (https://github.com/godotengine/godot/issues/63914).

Thanks!

authors: YuriSizov akien-mga


text:
Improve the interaction between SubViewport and SubViewportContainer

Follow-up to #64138, split from #62854.
SubViewport has some settings applied on initialization, and the side effects of these settings are delayed until it enters the tree.
Edit:
Handle the size_changed signal of SubViewport when SubViewportContainer adds/removes child nodes.
When the SubViewport re-enters the tree due to moving in the tree, update_mode is set according to the is_visible_in_tree() of the new parent SubViewportContainer node.
This patch makes SubViewport and SubViewportContainer work better out of the box.

Conceptually the changes make sense to me.
I am not sure, but maybe set_update_mode(SubViewport::UPDATE_ALWAYS); could be replaced by set_update_mode(SubViewport::UPDATE_WHEN_VISIBLE);?
In accordance with https://docs.godotengine.org/en/latest/community/contributing/best_practices_for_engine_contributors.html#prefer-local-solutions I believe, that it would be better to leave SubViewport::_notification as it is and change SubViewportContainer::add_child_notify and SubViewportContainer::remove_child_notify instead.
When looking at other locations, that connect to the size_change-signal, they use callable_mp instead of Callable, which doesn't rely on Strings for the called method. That seems to be less error prone.

@Sauermann  Yeah, it is more appropriate to put the signal connection in add_child_notify()/remove_child_notify(), but the setting of update_mode still needs to recheck the visibility of SubViewportContainer in the tree when entering the tree.

the setting of update_mode still needs to recheck the visibility of SubViewportContainer in the tree when re-entering the tree

Yes, I agree that this is necessary. My suggestion was to recheck the visibility also in SubViewportContainer::add_child_notify, so that it doesn't need to be set in SubViewport::_notification:
void SubViewportContainer::add_child_notify(Node *p_child) {
	SubViewport *c = Object::cast_to<SubViewport>(p_child);
	if (c) {
		if (is_visible_in_tree()) {
			c->set_update_mode(SubViewport::UPDATE_ALWAYS);
		} else {
			c->set_update_mode(SubViewport::UPDATE_DISABLED);
		}
[...]

Maybe the SubViewport has been parented to a different SubViewportContainer node with different visibility.

In that case the above code would have been executed for this different SubViewportContainer.
Here is my MRP for testing these changes: BugViewportInit.zip

Well, makes sense. This is related to reparenting, and add_child_notify happens after entering the tree. Thanks for the correction.

I'm not sure about this change, this should probably a separate setting that governs this behavior.
For instance, in VR it's often the setup where a stand alone subviewport is used that outputs to the HMD.
A second subviewport in a subviewport container is then used to create a spectator view (3rd person view). This viewport is often throttled by managing the viewport mode and toggling it to update once ever 3rd frame.
If that setting is overridden by this behavior that may break peoples code.

Yes, the switching of render_target_update_mode is too frequent and mandatory. Probably the desired mode before was UPDATE_WHEN_PARENT_VISIBLE, but it wasn't available.

Changes may affect the use in some special cases. While I can't test, it's prudent to close.

authors: BastiaanOlij Rindbee Sauermann


text:
[macOS .NET] Add homebrew paths to the search locations.

See https://github.com/godotengine/godot/issues/73747

I think there is another scenario that should not be overlooked.
If .NET is installed using the dotnet-install.sh script provided by Microsoft, it will be installed by default in the $HOME/.dotnet directory.
Therefore, I suggest that we should also search this directory additionally.

I found this topic after installing dotnet 9 through homebrew, forgetting that I had used the microsoft installer previously. I think this would be still valuable and not really obsoleted by either loading an env from zsh or by having an editor option for where to look for dotnet, considering that these are standard paths and that not everyone uses zsh. I rebased + built this locally and it worked as expected.

Closing in favor of #96146 + #81266 (this combination should work with any type of installation).

authors: a-johnston bruvzg zengxs


text:
[iOS one-click] Add support for Xcode 15 devicectl.

Fixes https://github.com/godotengine/godot/issues/85539
ios-deploy is still supported (for older devices / Xcode versions), but only active if path was explicitly set in the editor settings.

Thanks!

Cherry-picked for 4.2.2.

authors: YuriSizov akien-mga bruvzg


text:
Add interface/theme/{window_border_margin,top_bar_separation,default_margin_size} editor settings

This PR adds interface/theme/window_border_margin, interface/theme/top_bar_separation and interface/theme/default_margin_size editor settings in order to easily edit the editor interface margins.

[!NOTE]
This PR doesn't directly add a way to change the tree items margin size, only the default margin size used for all sorts of themes initiated during the startup for the editor.

Preview

TreeView (impacted by the change change)

default_margin_size at 4
default_margin_size at 2

Current issues with the PR

[ ] Some UI break with the default margins lower than 4 (like the Tree arrow placement and lines)

Fixes
Fixes godotengine/godot-proposals#8505

I think it makes sense to reduce the default spacing between docks (and the outer editor margin) as it doesn't really serve much of a practical purpose, but we don't need to add a setting for this.

As mentioned in the proposal, I don't think we should be exposing so many properties. If you want to customize the editor theme so granularly, you can already do that using the theme editor.
We are not exposing every property here, this is only a few that could be considered independent from the current theme. You could be using a theme to style it in some way and still prefer to tweak some values on top of it. Same as when users modify the text size or the rendering scale. So it is a bit of a strawman here. We are not adding every option the theme has to the panel, we are adding one that was requested and can benefit any theme you might already be using without having to modify it.

I also have plans for making custom editor/default themes based on the same parameters we use to do it in code, but exposed nicely to users. I feel like this will cover most of these edge case requests.
It is great that you have plans to do this, but right now we have this PR which would fix this. Do you have an estimate on when you'll be able to make a PR that implement this in a way that you think it's better?

I don't want this to be a "there can be a better way" hence we do nothing about it and we leave the PR, the proposal and the benefit of the user to rot.

@coppolaemilio

We are not adding every option the theme has to the panel, we are adding one that was requested and can benefit any theme you might already be using without having to modify it.

At least two of these properties are very specific and don't really benefit most of the themes as independent properties. I explained above that we don't have to expose properties one to one to their theme definitions. There is no such requirement. We can be smart about it and use a generalized and simple setting that would adjust all the properties described here. Or we can use the existing one that already works in a similar way by modifying it. Or we can just reduce margins by default, as Calinou suggests.
There are more options offered in just these few comments above than "we expose all or nothing". But if you insist on exposing them exactly as they are in the theme, then we do go back to the question raised in the proposal: where do we draw the line? How do you assess what's reasonable and what's not? Two people asked for two different things, and this PR does as it was bid by those two people. They have different amount of public support and are questioned on the merits differently. So what's the criteria that is not a strawman to you?

It is great that you have plans to do this, but right now we have this PR which would fix this.  ... I don't want this to be a "there can be a better way" hence we do nothing about it and we leave the PR, the proposal and the benefit of the user to rot.

And I don't want this to be a "Someone made a PR so we're already past the point of discussion, let's just merge whatever". I understand that you don't want to get stuck in a limbo discussing these changes, but that doesn't mean we shouldn't have a discussion at all. This is not how we work, and you are not in a position to decide for everyone if this is a good change or not. I feel slightly offended that key stakeholders like Calinou and myself are not taken seriously, like we're just hurdles in your path forward with this.
If you want to spend Adam's time on these improvements, why not spend it on a solution that we agree upon and see merit in? You don't have to get a consensus from the entire contributor base, but you could at least discuss it with the team before deciding that, yes, this is exactly what we need. Adam can just as well execute the plan that I imagine in my place, if we decide that it's a better solution. There is no need to commit to something hastily and then get defensive about it. We can just talk first, you know?

It's great that you feel passionate about this change!
What I see here is that there was:
problem -> solution a
And then, when presented with the solution, you say:
problem

solution b
solution c
solution d

So without going into the realm of: let's take this personal and fight about process.
What would your ideal solution be? I still don't know what you think should be done here.

My 2 cents:

it can expose a setting that is responsible for overall padding in various elements. We'd use it for the default margin and use twice as much for window border and the top bar separator

I'm fine with one setting that drives all of the paddings. I don't expect editor settings to provide too much granularity. As long as it affects the tree views enough to make the editor more practical, and if it's not hard to implement, I think it should be totally fine

we can discuss if it maybe should support negative values to reduce spacing in places

tbh to me as a user it doesn't really matter what range the values on a slider are as long as it does the thing. but I want to say that if it's going to be "extra spacing" it probably needs another name because to me as a user extra spacing that goes into negative feels like some kind of a hack

I think it makes sense to reduce the default spacing between docks

I agree with Calinou. None of the other apps that I use have dock gaps this big. I'd just cut them in half

I don't want this to be a "there can be a better way" hence we do nothing about it and we leave the PR

I agree with this too. This seems like a pretty simple and straightforward change, would be sad if it wasn't implemented due to some small disagreements on details, considering how many people expressed their interest in this

This seems like a pretty simple and straightforward change, would be sad if it wasn't implemented due to some small disagreements on details

So far nobody said that this shouldn't be implemented.

But why are you blocking then?

Because I want it to be implemented better. Above you can find a few suggestions about doing this change properly, including reducing default margins for everyone (which should already help most people who agree with @passivestar's proposal) and adding one general setting to control these margins instead of multiple settings.
Just because we defined independent properties in the editor theme doesn't mean we need to make it more finnicky to the end user to configure in the editor settings. The end user probably doesn't need to fine tune each individual property. They just want a more compact style for the editor in general. Please, don't give too much credit to the decision to use 3 different theme constants in the editor theme in the first place. It wasn't a conscious decision with a lot of weight on it, it was just a quick and convenient way to do it internally. So we don't have to abide by it. We can simplify and make it more sensible, more easily controlled, more high level.

and could already be merged

Just because someone made an effort it doesn't mean we are going to merge the PR. That's not how this project is governed. This is why we ask everyone to open proposals and discuss changes before committing to them. Making a PR is not a guarantee that your work is going to be accepted. And the proposal that this closes has an unresolved discussion, which now spills over here.

Making a temporary or partial solution in anticipation for a future, more comprehensive, solution just makes more work for everyone, the old feature has to be integrated, the old setting has to somehow be integrated with the new system, and users have to suddenly switch how they do things
Do don't want partial or temporary solutions for anything but critical issues that needs to be solved now

What right do you have to block other people’s proposals just because you want to do it differently?

As a stakeholder and a maintainer of the area it's my literal job to make sure contributions satisfy user needs, quality standards, and the overarching design.

@AsimDerth I have to remind you that we have a Code of Conduct. Please stay constructive.

This is an important feature for an usability standpoint that makes sense be available outside theming, someone working on a touch device would like to have more spacing just like a desktop user would like to have a more compact. But i understand what @YuriSizov is saying, having too many properties exposed could be detrimental to the user experience.
Having predetermined values could be a better experience, while allowing users to set a custom value using themes. One good example of that is Firefox

(ignore the compact version being deprecated)
They have a field called Density, with different options optimized for different use cases like Touch
But with custom css themes, you can set any value you want
We could do something like that, have a field called "Density" with multiple values like Compact, Normal, Tablet, etc with predetermined values optimized for that use case, but if the user would like to have a custom value, they could go with the custom theme route
(this is my first time posting and working with Godot, sorry for any mistakes i made)

I agree that this should be better implemented. Exposing 3 properties for only one purpose of reducing the gap between panels is so confusing and unnecessary complicated the UX, where the Settings already has the Reduce border size option which I thought it as an option to decrease the panel margin when the first time I’m using Godot. All other apps (like Blender, Unity…) have a very compact UI with small margin so why don’t we do the same? Just make it default by changing the hardcoded 8 to 1 or something, most user just go with it anyway because other popular apps do.

Personally, I don't want to see all the three settings in settings page for to achieve this requirement(which I very much want it)
But to achieve the requirements these three values are required to be configured.
I doubt and have a feeling that these are not the only ones that needs to be configured. There may be more values required to be configured.
The option of using themes to configure is bad, as already theme is too complicated for a basic user, and touching the theme used by the Editor 😨
My suggestion will be using a user-friendly setting like display_mode, which will have values like Default/Compact/Touch.
Internally Godot may use config files for each option containing all the settings required to achieve it. For advanced users they can edit the config files to further customise.

Superseded by https://github.com/godotengine/godot/pull/87085.

authors: AThousandShips Calinou TiagomCoutinho YuriSizov adamscott akien-mga chutneyio coppolaemilio passivestar tokengamedev


text:
Wayland: Setup next cursor frame callback only if animated

Before, the cursor kept updating for no good reason really.
It's also a bit neater and it ever-so-slightly makes WAYLAND_DEBUG logs easier to read, although they're still spammed by the window's frame logic (which is needed).

Thanks!

authors: Riteo akien-mga


text:
Fix some translations not properly falling back

Fixes #50650.

Thanks!

"Cancel" buttons in Project Manager dialogs are still untranslated. I guess this only fixes part of the problem.

authors: YeldhamDev akien-mga timothyqiu


text:
contrib/trace/otlpgrpc: grpc connection to opentelemetry collector authentication failed

Go version
go1.20
GoFrame version
2.7.0
Can this bug be reproduced with the latest release?
Option Yes
What did you do?

使用 github.com/gogf/gf/contrib/trace/otlpgrpc/v2 上传数据到 OpenTelemetry Collector，
当我给Collector 的 receivers 添加认证时，如 basicauth 或 bearertokenauth
如配置

extensions:
  health_check:
  bearertokenauth:
    token: "your-secret-token"

receivers:
  otlp:
    protocols:
      grpc:
        auth:
          authenticator: bearertokenauth
...

然后使用
otlpgrpc.Init(serviceName, ""localhost:4317"), "Bearer your-secret-token")

这将会报告认证失败 traces export: rpc error: code = Unknown desc = authentication didn't succeed
为什么会这样？
经过一天的排查我发现了问题原因
在例子中 https://github.com/gogf/gf/blob/master/example/trace/otlp/grpc/main.go 这里使用的阿里云
同时根据阿里云文档 https://help.aliyun.com/zh/arms/tracing-analysis/connect-opentelemetry-to-tracing-analysis 和 https://help.aliyun.com/zh/opentelemetry/user-guide/connect-to-managed-service-for-opentelemetry-and-authenticate-clients
这里的认证头是使用 Authentication 实现的
所以在 https://github.com/gogf/gf/blob/master/contrib/trace/otlpgrpc/otlpgrpc.go#L60 固定了hander
但是在 OpenTelemetry Collector 中是用 authorization 来接受认证头的，所以这导致认证失败
虽然只找到这个说明 https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configgrpc/README.md 和 https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configauth/README.md
但我实测确实是用  authorization  能认证通过
期望这个能调整这个内容，或者在例子中额外说明，这个只适用于阿里云而可能不适用于其他接收器
What did you see happen?
报错
traces export: rpc error: code = Unknown desc = authentication didn't succeed
What did you expect to see?
不应该报错

OpenTelemetry Collector  中获取 token hander 的相关代码 https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/extension/bearertokenauthextension/bearertokenauth.go#L187

建议参考自定义一个 OpenTelemetry client。 https://github.com/gogf/gf/blob/master/example/trace/provider/grpc/main.go

@houseme 我知道可以自己实现，但我更希望至少在文档上说明清楚，避免后续的人踩同样的坑

@houseme 我知道可以自己实现，但我更希望至少在文档上说明清楚，避免后续的人踩同样的坑

要不辛苦您补充一下文档内容，感谢🙏

authors: houseme ivothgle

repo: gogf/gf


text:
Update gtree_btree.go

bugfix: avoid overflow

Codecov Report

Merging #982 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #982      +/-   ##
==========================================
- Coverage   69.36%   69.34%   -0.02%
==========================================
  Files         350      350
  Lines       24691    24691
==========================================
- Hits        17126    17123       -3
- Misses       6152     6154       +2
- Partials     1413     1414       +1

Impacted Files
Coverage Δ

container/gtree/gtree_btree.go
88.40% <100.00%> (ø)

net/gtcp/gtcp_conn.go
32.06% <0.00%> (-0.77%)
:arrow_down:

container/gtree/gtree_redblacktree.go
87.89% <0.00%> (-0.42%)
:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 176dcdc...1072ea3. Read the comment docs.

authors: codecov-io lutherlau


text:
/install screen shouldn't accept the SMTP host without the port

Because later gogs fails to send an e-mail: "missing port in address"

For whoever wants to help, do a regexp check would be sufficient.

Could be solved in #2777

Patch has pushed to fix this issue, please test on develop branch.

authors: Jonny007-MKD Unknwon yurivict

repo: gogits/gogs


text:
Sometimes the deletion of Harbor cluster is a little slow

It seems the cluster cr deletion is blocked by the deletions of PVs

status:
  conditions:
  - lastTransitionTime: "2021-04-26T06:21:32Z"
    message: All resources successfully discovered
    reason: ResourcesDiscovered
    status: "False"
    type: NamespaceDeletionDiscoveryFailure
  - lastTransitionTime: "2021-04-26T06:21:32Z"
    message: All legacy kube types successfully parsed
    reason: ParsedGroupVersions
    status: "False"
    type: NamespaceDeletionGroupVersionParsingFailure
  - lastTransitionTime: "2021-04-26T06:27:55Z"
    message: 'Failed to delete all resource types, 1 remaining: unexpected items still
      remain in namespace: cluster-sample-ns for gvr: /v1, Resource=pods'
    reason: ContentDeletionFailed
    status: "True"
    type: NamespaceDeletionContentFailure
  - lastTransitionTime: "2021-04-26T06:21:32Z"
    message: 'Some resources are remaining: pods. has 7 resource instances'
    reason: SomeResourcesRemain
    status: "True"
    type: NamespaceContentRemaining
  - lastTransitionTime: "2021-04-26T06:39:36Z"
    message: All content-preserving finalizers finished
    reason: ContentHasNoFinalizers
    status: "False"
    type: NamespaceFinalizersRemaining
  phase: Terminating

kubectl patch pvc pgdata-postgresql-cluster-sample-ns-harborcluster-sample-1 -p '{"metadata":{"finalizers":null}}' -n cluster-sample-ns

repo: goharbor/harbor-operator


text:
Implement delta snapshot consumption/generation

We have the code to generate and consume them but we don't actually.

Implemented with #764

authors: luca-moser muXxer

repo: gohornet/hornet


text:
Enable responsive images for default markdown image markup

Hi,
I absolute do not know whether this is technically possible.
Hugo’s image processing is really cool. It would be extremely helpful, if an image/figure shortcode could be connected to markdown’s image handling.
Maybe there could be some default shortcode for this, and
![Alt text](image.jpg)

would become.
<img sizes="100vw" srcset="
/image_480x0_resize_q70_box.jpg 480w,
/image_768x0_resize_q70_box.jpg 768w,
/image_1024x0_resize_q70_box.jpg 1024w"
src="image.jpg" alt="Alt text">

Maybe it would even be possible to choose a specific shortcode by e.g. referencing its name before |:
![name-of-image-shortcode | Alt text](image.jpg)

This would kind of ‘free’ image handling from the Hugo specific shortcode and make images accessible using other markdown parsers (then, of course, just the original size). You could then easily convert your website to PDF, EPUB etc.

I have been thinking a bit. Writing a very specific shortcode is not very userfriendly.
So my suggestion ist just this:
config.yml:
enableResponsiveImages: true
responsiveImageWidths:
  - 500
  - 750
  - 1000

Usage:
![Alt text](image.jpg "My optional title")

Image sizes should not be increased. If your image is 300px wide just use it as a non responsive image. If the image is 800px wide use these widths (according to the settings above):

500px
750px
800px

I updated the title. Feel free to adjust. If ok I can delete my first issue comment.

repo: gohugoio/hugo


text:
Index Pages in the cache by their Title, if available

Fixes #4118
By adding pages to the cache index keyed by Title, I can look up an existing Taxonomy Term page by the un-sanitized term (in the same way that auto-generated Taxonomy Term pages can be looked-up).

Per notes in #4118 , this fix is inadequate for what I intended, so I'll revisit/reopen it later if appropriate.


text:
Send ride info when the ride starts or stops

When the ride starts, send a POST to /rideInfo creating a new ride with status 'ongoing'. When the ride finishes, send PUT to /rideInfo to update the status as 'finished'.

This is the schema I am using as of now:
{
"rideId"            : Number,
"status"            : String, //inprogress, complete
"startTime"         : Number, //time when the ride started
"stopTime"          : Number, //time when the ride ended
"analysisInfo"      : {
"status"          : String, //complete, incomplete
"timestamp"       : Number, //time when the analytics was last done
"rideSafetyScore" : Number  //Final safety score for this ride
}
}

Slight correction to the schema
{
  rideId: Number,
  status: String (enum: inprogress, complete),
  startedAt: Number,
  endedAt: Number,
  analysis: {
    status: String (enum: inprogress, complete),
    completedAt: Number
  }
}

We can have more information on rides and users in our MAC servers.
More info about using enums in mongoose http://mongoosejs.com/docs/api.html#schema_string_SchemaString-enum

Actually your idea work better....
{
  rideId: Number,
  status: String (enum: inprogress, complete),
  startedAt: Number,
  endedAt: Number,
  analysis: {
    status: String (enum: awating, inprogress, complete),
    version: Number,
    startedAt: Number,
    endedAt: Number
  }
}

Closed in #12

license: mit

license_source: bigquery


text:
time: LoadLocation doesn't work on windows if Go is not installed

What version of Go are you using (go version)?
go version go1.8.3 windows/amd64
Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (go env)?
set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\Iman Akabi\go
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
set CXX=g++
set CGO_ENABLED=1
set PKG_CONFIG=pkg-config
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
What did you do?
https://play.golang.org/p/VuJ3ofkdk8
package main
import (
"fmt"
"time"
"log"
)
func main() {
var ParisLocation, err = time.LoadLocation("Europe/Paris")
if(err != nil) {
log.Fatal(err)
}
fmt.Println(ParisLocation)

}
I built this basic go code and generated an executable file.
I have an error when I run this executable on a windows machine without Go installed because time.LoadLocation needs to aceess to "zoneinfo.zip" located at $GOROOT/lib/time/zoneinfo.zip.
What did you expect to see?
Europe/Paris
What did you see instead?
open c:\go\lib\time\zoneinfo.zip: The system cannot find the file specified.

I don't see how we can fix this, at least not without bloating the size of almost every Go binary on Windows.  I'm open to suggestions.

On windows couldn't it (default to|try) the (current working directory|executable location) to load the zoneinfo.zip? That way you only can include the file when it is required. Yeah, it's an extra hoop to jump through for us windows users, but I guess it's not too big of an ordeal seeing as the executable has to be distributed somehow, one extra file might not be too bad...

On windows couldn't it (default to|try) the (current working directory|executable location) to load the zoneinfo.zip?

Sounds simple enough for me.
Alex

zoneinfo.zip is a mere 366776 bytes, a small number amongst friends. I think of a typical go binary as a 10MB +/- 5MB affair, and I would happily increase that by 366KB (3% of the mean, but small compared to the standard deviation) to have my binaries be portable to windows.

I don't see how we can fix this, at least not without bloating the size of almost every Go binary on Windows.

Providing the tzdata ourselves in the binary if a specific package is imported (e.g. time/zone) seems less worse to me than requiring every Windows user to install Go, which is much larger to download and is inconsistent with the static binary.
Could we duplicate the time.LoadLocation function to another package which also contains the tzdata, e.g. zone.LoadLocation. This new function would operate the same as time.LoadLocation, but would load from a tzdata file built into a new zone package. This would only introduce bloat to binaries that imported the zone package, and applications could choose to use time.LoadLocation or zone.LoadLocation. We could mark time.LoadLocation as deprecated, or at least mention in a comment on it about it's limitations and about the alternative. Having the basic time code and the zone code in separate packages would mean the bloat is only introduced if applications specifically need it.
This would probably also resolve issues #20629 and #20969.

Just an idea. On windows we use a particular Windows registry key to determine our timezone, and then we use a map that maps that string into std / dst values that are ultimately used to look in the tzdata blob. The "registry key" to "std / dst" map is hard coded in "abbrs" map in time package. This map contains 133 values at this moment. I wonder if we could strip tzdata blob to leave only the data we actually use on windows. I don't have time to try and implement this now, perhaps others do.
Alex

I was pretty astonished by this. I can trivially build and deploy binaries to Windows just like I can everywhere else, but as soon as I try to parse a time, my application blows up. Go's tooling lulls me into a false sense of ease upon which I grow to rely, but it's a false promise. I'd much rather a larger binary than an... ahem... time-bomb like this ticking away.

All, what's the best current workaround for this?  I need to distribute a windows binary, that uses time, and I'd like it not to blow up on user's boxes.
import "time/zone" would be great, eventually. But what's the solution today?
Thank you!

The solution is to ship the tzdata yourself (linked into your binary or elsewhere) and then call https://golang.org/pkg/time/#LoadLocationFromTZData.
Or set the %ZONEINFO% environment variable to point to your shipped zoneinfo.zip.

Thanks Brad.
For others, the data that is passed to LoadLocationFromTZData is a file extracted from inside zoneinfo.zip, such as the file America/New_York, not zoneinfo.zip itself.

This is the quickie solution I used yesterday to get moving again: https://gist.github.com/shabbyrobe/b9a7a56e25bd8e441b7b3fe39dbb04fa
It relies on something not unlike the now-infamous https://github.com/jteeuwen/go-bindata to generate a go file with that tzfiles map referenced in the gist, which contains the base64-encoded zoneinfo.zip file I grabbed from the GOROOT of another Windows machine that was allowed to have Go installed on it (not always an option).
The code was dashed out under the duress of extreme time pressure so it's not exactly nice (and comes with absolutely no warranty whatsoever) but hopefully it can help someone else get out of the same jam.

I created 4d63.com/tz because I find it more convenient to import a package than manually including a file to ride alongside an executable. It embeds Go's zoneinfo.zip and exports tz.LoadLocation(name) to load locations from the embedded tzdata.

Awesome @leighmcculloch. That should really become part of the standard library, but while waiting for that, its just great to have it available.  In the meantime, all calls to time.LoadLocation, in order to not crash on windows/OS without zoneinfo, need to become calls to tz.LoadLocation() after
import "4d63.com/tz".

I just spent 4 days debugging why my DLL would load on one windows host and crash on another. Time I won't ever get back. Yep, it was due to missing the %GOROOT%\lib\time\zoneinfo.zip file on the deploy host.  Pretty please include zoneinfo.zip with the Go runtime on windows. Windows DLLs can't be distributed to non-developers as it stands in go1.12.5.

@glycerine Sorry you spent so much time on that, agreed it's not the best solution, but in reading the discussions related to time zones on windows, adding useless static data to every go program on windows (especially when it's not required) isn't a great solution either. Windows being the oddity because it does time zones differently. You're really left with one of the above solutions, distribute the zoneinfo.zip file with your app and explicitly load it, or set the %ZONEINFO% environment variable to point to your shipped zoneinfo.zip. I had similar issues nearly 2 years ago, but now, I know the fix and to me it's just a windows(ism) I remember to always have to do. Also it came up more recently when I was building scratch containers for Linux too along with a few other dependencies under the hood. I guess also using https://4d63.com/tz work too, possibly a much cleaner solution.

adding useless static data to every go program on windows (especially when it's not required) isn't a great solution either

It's less worse though.

adding useless static data to every go program on windows (especially when it's not required) isn't a great solution either.

I must vehemently disagree. This is the preferred solution. My Go binaries are 50MB.
My gosh.  The timezone data is a tiny file, less than 400KB. Just add it. Good grief. Every program of any complexity will import the time package, and want to not crash on windows.
Sample fix, needs the niceties of review and convention applied, but for those of us who needed a solution yesterday:
https://github.com/glycerine/go/tree/timezone_windows_fix

The timezone data file is nearly 800K now, due to #30099.  Not that that really affects this calculation.

There are a couple competing things for me about this issue:
An attribute of Go binaries is that they are static, containing all their Go dependencies, so tzinfo not being included is a surprise. And, in the Go community using tools like packr, embedfiles, it is common to pack files into binaries rather than ship them sidecar, giving the sense people prefer things baked in.
But, TZ data is more like certificate authorities than application data, and we don't bundle certificate authorities into Go binaries because they are typically managed by sysadmins. Sysadmins probably care about TZ data less though making the utility of them being updatable/changeable less useful?
Since Go only pulls in packages if they're imported could we have tzinfo in a separate package that is only pulled in if it was required? Would it be possible to separate it from the time package? I'm guessing no.

Add me to the list of people inconvenienced by this; philosophically my vote is for baking tz data into the binary given that the binaries are already relatively large.  Whatever the fix is, I am surprised this issue was reported in 2017 and still remains for such a seemingly big issue (affecting tz use on entire platform).

There are various ways to address this that are described above.
The question is: what should the default behavior be?  I agree that the current default is a poor choice.  But it's not obvious to me that growing the size of essentially every Windows Go binary by 800K is a better choice.  So perhaps we need a way to make the default be to make the binary larger but have some way to disable that.  But I don't know what the disabling mechanism would be.

So perhaps we need a way to make the default be to make the binary larger but have some way to disable that. But I don't know what the disabling mechanism would be.

The disabling mechanism could be importing a "thintime" package instead of "time".
It might be worth taking the time to redo the API for thintime.Location in this suggested thintime package.  If my suspicions are correct, the thing that really bloats the use of time.Location is that it uses strings as keys to pick a location. Since the compiler cannot know which strings will be used, it has to include all time locations. If it used something that the compiler could do dead-code elimination on (or the equivalent for static data), then my program wouldn't need all 800K of timezones. After all, I use but three locations at the moment, and would be happy to recompile if I needed others.

The alternative would be to reimplement large parts of this package, to use Windows time APIs directly instead of parsing a bundled tzdata.
I'm not sure if it's 100% feasible, but, clearly these APIs do exist:

.NET has a copy of tzdata for System.TimeZoneInfo, and Edge/IE also ship a copy of tzdata for the Intl implementation. But these might not be accessible from Go
boost::time_zone handles this somehow in C++
Windows 10 1709 added ucal_getDefaultTimeZone, ucal_openTimeZones etc in icuin.dll
WinRT/UWP has Windows::Globalization::Calendar().GetTimeZone() available, that can be called by COM
Use the classic Windows GetTimeZoneInformation C API and bundle in the (much smaller) mapping from IANA<-->Windows zone names

Actually while researching this, I discovered the %WINDIR%\Globalization\Time Zone\timezones.xml file seems to be a complete version of the tzdata database, using IANA names, in XML format :star:
From https://data.iana.org/time-zones/tz-link.html#CLDR i think this file was introduced in Windows 8.1. (As of January 2020 that will be the minimum Microsoft supported Windows version.)
I think adding support for parsing this file in time package would resolve this issue,

Changing the Windows logic to use %WINDIR%\Globalization\Time Zone\timezones.xml parallels what happens on Linux and Mac machines, by using the system values, so that sounds like it would make the most sense.
To preserve backwards compatibility would we need to have it still use Go's version if Go is installed, or would that be dangerous since developers running the app on the same operating system as a user who doesn't have Go installed may see different behavior?

I think adding support for parsing this file in time package would resolve this issue,

encoding/xml already depends on time, so making time depend on encoding/xml wouldn't be allowed. We'd need to do something tricky. Not impossible, but it'd be a little ugly.

I favor using the identical zonefile on all builds. I don't want to deal with Heizenbugs that only affect windows. The windows .xml files don't have the Daylight saving time (DST) rules, for example.

I favor using the identical zonefile on all builds. I don't want to deal with Heizenbugs that only affect windows.

Mismatched zone files happen on Linux too, since the system version is used, and distros may be using a different or outdated system version of the tzdata database.

The windows .xml files don't have the Daylight saving time (DST) rules, for example.

That's not correct, the DST rules are present from line 2872 in my installation.

Can someone show a sample of one of the files found on Windows?  Thanks.

Can someone show a sample of one of the files found on Windows? Thanks.

%WINDIR%\Globalization\Time Zone\ directory from Windows 1903: Time Zone.zip

Thanks.  It does look to me as though the Windows data has all the information that the time package cares about.  If that data is available on all or most Windows systems, it seems worthwhile to add support to the time package for reading it.  As @bradfitz says above, the time package cannot use the encoding/xml package, but the data seems easy enough to parse.
Unfortunately, the fact that it is all in one big file means that fetching the information for a specific timezone will be slower than when reading the Unix-style tzdata files.  But that is probably still a good tradeoff for Go programs running on Windows.

Would it be a short-term option to at least show a warning when compiling for Windows?  That could save a lot of people a lot of time while the team decides on a proper long-term solution...

I don't understand how that would help anybody.  The great majority of Go programs use the time package, but very few need timezone information for any timezone other than the one they are in.

Since this hasn't seen love since in a few months: I also have an application I need to distribute to Windows machines that very likely do not have the Go toolchain installed. My application uses LoadLocation.

@elagergren-spideroak I have to hack the Go runtime with every new release because of this issue. https://github.com/glycerine/go/tree/timezone_windows_fix has the template for for the fix. It uses the approach from https://4d63.com/tz of embedding the needed data in the time package.

zoneinfo.zip is a mere 366776 bytes

@glycerine Wait a minute, is this? Okay, if so: why not just use windows api? may be it is, idn, cause i working on unixes, or may be not, but i pretty sure that windows os has any timezone api and timezone data.
As i can see, problem is not solved on two platforms: windows and linux (that's why i am here🙃🙃🙃🙃). Perhaps, it necessary to reopen this issue, cause it can be fatal problem on delivery any go apps on both platforms. @ianlancetaylor

This issue is still open.  There is a comment above suggesting that we can read the data from Windows.  What needs to happen now is that somebody needs to write that code.

@ololosha228 Please open a separate github issue if linux is having issues too. This one is focused on Windows.  You can link to this if you think it is a similar issue, but I would guess it is different.

somebody needs to write that code.

We should just ship the "fat fix" of including the timezone file in the Go binary. Two+ years of this issue being open is depressing.
Later, as an optimization, windows binaries can be slimmed down if the 800KB becomes an issue. 100% prediction: it will never come up.

Including zoneinfo.zip in every Go binary can be done but it's not a five minute job.  The time package is at the bottom of the import hierarchy, and there are very few packages that it can import.  In particular, it can't import archive/zip.  So that code will have to be repurposed and slimmed down for use in the time package.
(And people do regularly complain about binary sizes, though perhaps not on Windows.)

No need to unzip it (that would be complex and slow), just include it directly. Here's the diff of my patch. It is only 25 lines. And it adds one additional file with the data.  Yes it is against an older version of Go, but the changes should still be trivial against head.
https://github.com/golang/go/compare/master...glycerine:timezone_windows_fix

@glycerine sure, i'll open new issue and try find way to fix it.
Btw, i have two questions:

why timezone data sized as chubby cat jpeg? I mean, 300+kb, for 24+ time zones... It's too large. But maybe i cam find explaination on stackoverlow, okay.
windows has timezone api, as i wrote above. May be for windows (not unix) it can be really good fix? Good reason is: you don't need delivery go dependency as timezones.zip (or how is it name).

I'll open issue for linux, and try to find same api on these weekends.

@glycerine What is the effect on binary size of your patch?

@ianlancetaylor I'll look when I'm back at the desk.

One of my DLLs goes from 68,247,552 bytes -> 67,714,048 bytes when the patch is removed. This is 0.7% change in size, a 533,504 byte difference.
Even if it was double that, since you mentioned earlier that might have happened in a subsequent addition, the fact that the binary can now RUN instead of inexplicably not run, it is so worth it.
Using _windows.go file extension, this could also be made windows-only.

What do people think of the approach in https://golang.org/cl/224588?  With that CL, any program that imports the new time/tzdata package (as import _ "time/tzdata") will get an embedded copy of the tzdata database.  This will let the program work if the system does not have any timezone information.  The program will first check the timezone information on the system, as they may be more up to date.  If it doesn't find anything, it will use the embedded copy.  This increases the size of the program by about 800K.

That sounds like a very pragmatic solution, thanks Ian.

I think it should be up to the individual who deploys the binary to ensure tzdata is present.
With the _ import, if some library you depend on imports the tzdata package, then you're forced to take the 800K binary growth even if you plan to deploy with tzdata and would never use it. I can imagine some obscure library importing this ("we need timezone info, make sure we have it") and then imports who are unaware of this start growing as a result.
I'd personally rather see a flag to tell the compiler to include it than a package I can't opt-out of.

What do people think of the approach in https://golang.org/cl/224588?

@ianlancetaylor I think that sounds great. Thank you for putting that together.

I think it should be up to the individual who deploys the binary to ensure tzdata is present.

This is not realistic/too technical for a customer who installs your software. Welcome to the retail Windows software world.

This is not realistic/too technical for a customer who installs your software. Welcome to the retail Windows software world.

This is not what I meant. I meant at build time, if you know you need it, you can include it in the binary. I am not saying that it's something else that needs to be installed; that is the current solution.

With that CL, any program that imports the new time/tzdata package (as import _ "time/tzdata") will get an embedded copy of the tzdata database. ... This increases the size of the program by about 800K.

This will effectively means that all my programs will become 800K larger. Some developers of some package I use in my program will decide to include "time/tzdata", and I have to carry this useless extra disk space. This does not scale.
Alex

I think the CL (https://github.com/golang/go/issues/21881#issuecomment-602133486) is an elegant way to let a developer choose to bundle backup tzdata, but I don't think it brings timezone support on Windows to feature parity with other operating systems that Go supports.
For example, if a developer working on Linux or Mac builds and ships binaries on all operating systems, as is common, they won't find out there's a problem until they get their first Windows user who reports an issue with the application having no timezones. So there's still the element of surprise and the code we non-Windows users write is still not Windows friendly by default.
Because of this I think parsing in that XML file idea (https://github.com/golang/go/issues/21881#issuecomment-552239335) is still the best solution that's been suggested as it brings the time package for Windows into parity with other operating systems. It also keeps tzdata where we expect it, in the operating system.

Some developers of some package I use in my program will decide to include "time/tzdata", and I have to carry this useless extra disk space.

No, you are not impotent in this scenario. Simply delete the import from the library.
I doubt this will pacify these voices, but let's add to the time/tzdata package documentation the convention that only the main package should be importing time/tzdata.
That way anyone can feel free to delete such an import if they find it in a library. No guilt.

@leighmcculloch I think the time/tzdata suggestion is independent of adding support for parsing the Windows XML file.  I think we should do that too.  But even then there would be a use for some people for embedding the tzdata information in the binary itself.

What do people think of the approach in https://golang.org/cl/224588?

Yes, this is awesome platform-independent solution, i like it. Technically, it can be more graceful, but pragmatically, looks pretty.
But. I wonder that if this solution will release, some guys will describe it as default solution, not problem specific. May be it is good to also edit docs? I can open pull request maybe.

I've filed #38017 as a proposal for a time/tzdata package.  Please comment or emoji vote there about this idea.  Thanks.

I think the time/tzdata suggestion is independent of adding support for parsing the Windows XML file. I think we should do that too.

@ianlancetaylor I agree, sounds good! 👍

But even then there would be a use for some people for embedding the tzdata information in the binary itself.

For the use case of someone choosing to embed tzdata into their app, and not for solving this specific problem with Windows users, does it still make sense for the embedded tzdata to be only a backup? If I am choosing to embed data into an app I expect that data to be authoritative and be the only data used. I can see both cases (backup vs authoritative) being appropriate to different situations. Could we make the time/tzdata work like certificate pools and root CAs work? Any app can embed its own set of root CAs by creating a cert pool and adding that cert pool to the http package's default transport. The Go stdlib could still include the tzdata package, but I think the way it gets hooked up to the time package should allow:

Making it a backup source of tzdata
Making it an authoritative source
Specifying a custom source (unlikely to be used?)

@leighmcculloch Can you discuss that over on the issue I just opened, #38017?  Thanks.

Sorry @ianlancetaylor I think we posted about the same time. I'll post over on #38017.

Re-opening as CL 224588 was rolled back in CL 228200.

As per https://github.com/golang/go/issues/21881#issuecomment-602269937 , the new package in CL 228101 is not a fix for this issue - just a workaround that is not suitable in all cases.
Please re-open this ticket,

I believe that the new time/tzdata package is a fix for this issue.  And this issue has gotten lengthy and complex with 65 comments (now 66).  Rather than reopening this issue, I encourage you to open a new issue for reading the XML file on Windows.  Thanks.

@mappu if you do, please link it here, so that those who are interested could follow

@mappu opened the new issue at #38453.  Thanks.

authors: AndrewSav alexbrainman bradfitz dmitshur elagergren-spideroak glycerine ianlancetaylor iman75 jfritz leighmcculloch mappu ololosha228 shabbyrobe skirmish thebitguru zikaeroh

repo: golang/go


text:
cmd/go: duplicate vendored packages listed by go list -f {{.Deps}}

go version devel +8f7805f9ce Tue Jan 30 13:43:42 2018 +1100 darwin/amd64

I ran this command and got this output:
% go list -f '{{join .Deps "\n"}}' upspin.io/transports  | sort -u  | grep bidi
upspin.io/vendor/golang.org/x/text/secure/bidirule
upspin.io/vendor/golang.org/x/text/unicode/bidi
vendor/golang_org/x/text/secure/bidirule
vendor/golang_org/x/text/unicode/bidi
%

It may just be a reporting problem, but I find it troubling that the same packages are listed twice, with the vendor/... ones not even being legal import paths.

That's correct output.
You have two copies of those packages in your program: the ones privately vendored by the standard library, and the ones you're using from upspin.
The illegal import paths are on purpose to work around a bug. #17326 is already open to track that.
Not sure there's anything to do here.

For some reason I checked to see if they were vendored by the Go repo and concluded they weren't; I clearly made an error. Closing.
Vendoring has got to go.

authors: bradfitz robpike


text:
sync: Map.Delete to return deleted value

I need to know if an item is actually deleted when call Delete of sync.Map, but the function returns nothing currently.
Is it possible for it to return the deleted value and/or a bool to indicate whether something is really deleted?
Thanks.

It is not possible to do that without breaking Go1 compatibility. I assume that you don't want to make an extra .Load call to check whether the element is present in the map or not ?

/cc @bcmills

@agnivade, thanks for the reply.  However, I don't think an extra .Load call works, because the item could be deleted in another go-routine between my call to .Load and .Delete.
I have little knowledge about Go1 compatibility, but in my understanding, add return value to .Delete won't break any existing code, because it currently returns nothing.

@localvar 'fraid not, the current intpretation of the Go 1 contract assumes that someone has assigned a method as a function; eg
var m sync.Map
var del = m.Delete

Changing the signature of sync.Map.Delete could break code like this, depending on how del is used, hence this type of proposal is not acceptable under the Go1 contract.

Duplicate of #23547 and closing as #23547 states that this change cannot be accepted.

Ah I was pretty sure this was raised before but could not find anything when I searched. Thanks.

Is adding a new function (for an ugly example: Delete2) acceptable?
I checked the source code and find the internal entry.delete does have a return value.

authors: agnivade davecheney localvar


text:
cmd/go: trim go.sum without adding module requirements

What version of Go are you using (go version)?
go version go1.11.4 linux/amd64
Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/user/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/d/14-Workspace/go/github.com/F21/somerepo/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build348706889=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Create go.mod: go mod init github.com/F21/somerepo.
Populate go.mod and go.sum by running go test ./....
One of the dependencies github.com/uber/jaeger-client-go conflicts with another dependency, so I bumped it to master: go get github.com/uber/jaeger-client-go@master.
There are now 2 entries for github.com/uber/jaeger-client-go in my go.sum:

github.com/uber/jaeger-client-go v2.15.0+incompatible h1:NP3qsSqNxh8VYr956ur1N/1C1PjvOJnJykCzcD5QHbk=
github.com/uber/jaeger-client-go v2.15.0+incompatible/go.mod h1:WVhlPFC8FDjOFMMWRy2pZqQJSXxYSwNYOkTr/Z6d3Kk=
github.com/uber/jaeger-client-go v2.15.1-0.20190116124224-6733ee486c78+incompatible h1:b2A4zBpOpzqZoSlqlJ3OOe0yjd577TnQIHG1/KN7U8E=
github.com/uber/jaeger-client-go v2.15.1-0.20190116124224-6733ee486c78+incompatible/go.mod h1:WVhlPFC8FDjOFMMWRy2pZqQJSXxYSwNYOkTr/Z6d3Kk=

The github.com/uber/jaeger-client-go v2.15.0+incompatible entries are unnecessary as the project is now pinned to github.com/uber/jaeger-client-go v2.15.1-0.20190116124224-6733ee486c78+incompatible (current master).
In https://github.com/golang/go/issues/26381, the ability to trim go.sum was added to go mod tidy. However, go mod tidy is something I avoid as it pulls in all test dependencies for all my dependencies and their dependencies for all platforms. Unfortunately, go mod tidy has burnt me quite a few times and causes a lot more problems than it solves.
See:

https://groups.google.com/forum/#!topic/golang-nuts/5otdvVra0xg
https://github.com/golang/go/issues/27920

What did you expect to see?
I'd like to be able to trim unneeded entries from go.sum without having to run go mod tidy as its current behavior causes a lot more problems than it solves.
What did you see instead?
N/A.

It would be nice to have a way to cleanup go.sum from stuff that are not relevant for the building the module and only keep the minimal required.
The current behavior adds hashes for all the stuff in one's go root which one might not want to expose to others (privacy) when pushing the module to a repo. Right now I need to manually cleanup the go.sum file before pushing it to the repo.
relevant bug: https://github.com/golang/go/issues/26381

Any update on this? Am I right in understanding that there is still no way to trim hashes from go.sum which belong to packages that are removed from go.mod?

@qbiqing go mod tidy does that, in addition to adding missing requirements in go.mod and removing unnecessary requirements.
This issue is about having separate functionality to trim sums from go.sum that go mod tidy would add, including test dependencies of dependencies and modules that only provide packages on platforms that aren't supported by the module author.
Since this is a very narrow case, and since having additional hashes in go.sum doesn't have any build or run-time cost, we haven't prioritized it.

@qbiqing go mod tidy does that, in addition to adding missing requirements in go.mod and removing unnecessary requirements.
This issue is about having separate functionality to trim sums from go.sum that go mod tidy would add, including test dependencies of dependencies and modules that only provide packages on platforms that aren't supported by the module author.
Since this is a very narrow case, and since having additional hashes in go.sum doesn't have any build or run-time cost, we haven't prioritized it.

@ghasemloo

The current behavior adds hashes for all the stuff in one's go root which one might not want to expose to others (privacy) when pushing the module to a repo. Right now I need to manually cleanup the go.sum file before pushing it to the repo.

go mod tidy doesn't add sums for anything outside the current module and its dependencies. After running go mod tidy, go.sum contains sums for modules providing packages needed to build packages in the main modules, as well as tests of those packages, on any platform. It doesn't depend on anything in GOROOT or GOPATH outside the main module, so there should be no privacy issue. go mod tidy has more info.

@ghasemloo

The current behavior adds hashes for all the stuff in one's go root which one might not want to expose to others (privacy) when pushing the module to a repo. Right now I need to manually cleanup the go.sum file before pushing it to the repo.

go mod tidy doesn't add sums for anything outside the current module and its dependencies. After running go mod tidy, go.sum contains sums for modules providing packages needed to build packages in the main modules, as well as tests of those packages, on any platform. It doesn't depend on anything in GOROOT or GOPATH outside the main module, so there should be no privacy issue. go mod tidy has more info.

I wrote some code to do this.
I will upload to a github project and share the link.

here is the hacky way I did it:
gosumtrim/gosumtrim.go
// Binary gosumtrim trims the go.sum file based on the given go.mod file.
package main

import (
	"bufio"
	"flag"
	"fmt"
	"io"
	"os"
	"strings"

	"github.com/golang/glog"
)

var (
	modFile = flag.String("mod", "", "input go.mod file")
	sumFile = flag.String("sum", "", "input go.sum file")
	outFile = flag.String("out", "", "output go.sum file")
)

func main() {
	flag.Parse()

	modIn, err := os.Open(*modFile)
	if err != nil {
		glog.Exitf("Open(%q) failed: %v", *modFile, err)
	}
	sumIn, err := os.Open(*sumFile)
	if err != nil {
		glog.Exitf("Open(%q) failed: %v", *sumFile, err)
	}
	sumOut, err := os.Create(*outFile)
	if err != nil {
		glog.Exitf("Create(%q) failed: %v", *outFile, err)
	}

	if err := trim(modIn, sumIn, sumOut); err != nil {
		glog.Exitf("Trim() failed: %v", err)
	}
	if err := sumOut.Close(); err != nil {
		glog.Exitf("os.Close() fialed: %v", err)
	}
}

func trim(mod io.Reader, sum io.Reader, o io.Writer) error {
	m := bufio.NewReader(mod)
	s := bufio.NewReader(sum)

	// Read till we get to the require line.
	for {
		line, err := m.ReadString('\n')
		if err != nil {
			return fmt.Errorf("reading mod file failed: %v", err)
		}
		glog.Infof("line: %v", line)
		if line == "require (\n" {
			break
		}
	}

	// Process require lines and add dependencies into used map.
	used := make(map[string]bool)
	for {
		line, err := m.ReadString('\n')
		line = strings.Trim(line, "\n \t")
		if err != nil {
			return fmt.Errorf("reading input go.mod failed: %v", err)
		}
		glog.Infof("line: %v", line)
		if line == ")" {
			break
		}
		used[line] = true
	}

	glog.Infof("used: %v", used)

	// Filter sum->out, dropping what is not used in mod.
	for {
		line, err := s.ReadString('\n')
		if err == io.EOF {
			break
		}
		glog.Infof("line: %v", line)
		if err != nil {
			return fmt.Errorf("reading input go.sum failed: %v", err)
		}
		parts := strings.Split(strings.TrimSpace(line), " ")
		if len(parts) != 3 {
			return fmt.Errorf("expecting 3 parts per go.sum line, got %v", parts)
		}
		if !used[strings.Join(parts[:2], " ")] {
			continue
		}
		if _, err := o.Write([]byte(line)); err != nil {
			return fmt.Errorf("writing output go.sum failed: %v", err)
		}
	}
	return nil
}

gosumtrim/gosumtrim_test.go
package main

import (
	"bytes"
	"io"
	"io/ioutil"
	"os"
	"testing"

	"github.com/google/go-cmp/cmp"
)

const (
	root       = ".../gosumtrim/" // module root
	inModFile  = root + "testdata/go.mod.in"
	inSumFile  = root + "testdata/go.sum.in"
	outSumFile = root + "testdata/go.sum.out"
)

func TestTrim(t *testing.T) {
	mod := mustOpen(t, inModFile)
	sum := mustOpen(t, inSumFile)
	out := mustOpen(t, outSumFile)
	want, err := ioutil.ReadAll(out)
	if err != nil {
		t.Fatalf("ioutil.ReadAll(out) failed: %v", err)
	}

	b := bytes.NewBuffer(nil)
	if err := trim(mod, sum, b); err != nil {
		t.Fatalf("trime() failed: %v", err)
	}
	got := b.Bytes()

	t.Logf("got:\n%v", string(got))
	t.Logf("want:\n%v", string(want))

	if diff := cmp.Diff(string(got), string(want)); diff != "" {
		t.Fatalf("trime() returned:\n%s", diff)
	}
}

func mustOpen(t *testing.T, fn string) io.Reader {
	t.Helper()
	r, err := os.Open(fn)
	if err != nil {
		t.Fatalf("os.Open(%q) failed: %v", fn, err)
	}
	return r
}

https://github.com/ghasemloo/gosumtrim

authors: F21 ghasemloo jayconrod qbiqing


text:
Panic/Warn, don't silently truncate, copy() when destination buffer is too small

copy() silently ignores the case where destination is too small to contain the values from the source buffer. Can we extend the API to report an error for this, or at least panic()?

This is well documented: https://golang.org/pkg/builtin/#copy
In particular, it returns an int to report the number of bytes copied. You could easily write your checks with that.
We can't change its signature now, as that would be a breaking change. See https://golang.org/doc/go1compat. Feel free to write a Go2 proposal, but I imagine this change is far too small and unimportant to warrant changing the builtin package.

Checking integers is not something the Go ecosystem easily enforces on behalf of the unsuspecting programmer. Checking errors (errcheck) is.

authors: mcandre mvdan


text:
proposal: Go2 error handling for error-tree

Please add into wiki Go2ErrorHandling, my proposal for error handling in error-tree. Preview:
func printSum(a, b string) error {
  // create error tree
  et := errors.New("check input data")
  errorhandling(et interface{Add(error)}){ // add type just for clearification
    x, et := strconv.Atoi(a)
    y, et := strconv.Atoi(b)
  }
  // is error-tree have errors
  if et.IsError() {
    return et
  }
  fmt.Println("result:", x + y)
  return nil
}

In detail see: Go2ErrorTree

CC @jba @neild

It looks like there are two ideas here. One is an "error tree" type that can collect multiple errors. It seems that this type is itself an error, but it can also represent the state of there being no errors. So it is already incompatible with the basic property of Go error handling, that there is an error if and only if err != nil.
The second idea is some new syntax for automatically collecting errors into an error tree. That results in simpler control flow (no if err checks) for code where several actions are taken independently. I believe that is rare, however; it's much more common for an action to depend on the result of a previous action. In those cases, the special syntax will obscure bugs. For example, in this code:
errorhandling(et interface{Add(error)}) {
   a, et := f()
   b, et := g(a)
}

we erroneously pass a garbage a to g when f returns an error.

Yes, @jba, you are right. Syntax errorhandling is work fine, only if expessions inside are independancy.
And you example looks like(it is valid code see https://play.golang.org/p/xl5dRJgvKqU):
  a, err := f()
  // no error checking
  b, err := g(a)
  if err != nil{
    return err
  }

but in that case we will not see error from function f(). If we use errorhandling, then error will garantee added in error-tree.

@Konstantin8105 what are the problems and solution requirements that motivated your proposal?
FYI, I've collected a menu of Requirements to Consider for Go 2 Error Handling.
Also, please feel free to add a link to your proposal to the wiki:
https://github.com/golang/go/wiki/Go2ErrorHandlingFeedback

Now, I add to wiki https://github.com/golang/go/issues/32099.

Thanks for this proposal.  This combines error handling and error values into a single proposal.  But the idea is somewhat complicated: it requires a builtin type more complicated than error, and it requires additional control within the errorhandling construct.  It also seems to implicitly call an Add method, which is not something we ever do in the language today.
We aren't going to adopt this idea.

authors: Konstantin8105 ianlancetaylor jba networkimprov