525473 - symlink doesn't work on Windows, so mod_perl will recompile all templates

Status: ASSIGNED

(Bugzilla :: Bugzilla-General, defect)

Description

[Michael Thomas (Mockodin)]

Bugzilla::Template currently omits symblinking for windows when running under apache with mod_perl becuase it never worked properly. As of Windows Vista\2008 mklink provides the correct functionality. But Perl doesn't support it via symlink() yet.

This should be rolled in to the code but I'm providing it as a hack for those adventurous enough to go manually apply it.

In Bugzilla::Template search for "# Under mod_perl, we look for templates using the absolute path of the"

After the close of the next if block (if (!ON_WINDOWS) {}) add the following code snip:

#BEGIN SNIP
else {
        my $abs_root = dirname(abs_path($templatedir));
	# remove the : from the absroot
	$abs_root =~ s/://;
	my $todir    = "$datadir/template/$abs_root";

	mkpath($todir);
        # We use abs2rel so that the symlink will look like 
        # "../../../../template" which works, while just 
        # "data/template/template/" doesn't work.
        my $fromdir = File::Spec->abs2rel("$datadir/template/template", $todir);
	print "\n".`mklink /D "$todir/template" "$fromdir"`."\n";
}
#END SNIP

use of `` is the main part I refer to as a hack, a lot of folks don't like it. However it is functional and will provide the appropriate work around until Active Perl / Strawberry or other perl flavor makes the appropriate changes.

the other part is you;ll notice there is no check in my code for OS version. If you run Windows XP /2003 it should proceed without stopping checksetup however it will likely throw an error message in the output. Since `` is employed, eval {} will not catch the error.

Comment 1

[Frédéric Buclin]

But Bugzilla doesn't work with mod_perl under Windows. Why is this hack useful?

Comment 2

[Michael Thomas (Mockodin)]

Works fine for me? That it doesn't work with the default script doesn't mean it doesn't work.

All you have to do is disable Apache2::SizeLimit. Which while in theory might be useful, is less important under the windows Apache model which is threaded by default.

Is it possible to max out memory on a server without it sure. But you can put other limiters in to prevent it, such as MaxChildRequests

Comment 3

[Michael Thomas (Mockodin)]

Attachment: v1 for 3.5.2+

Patch against Bugzilla 3.5.2+

I expect it will need to clean/tweaking to make LpSolit and mkanat happy but it works as is. Test under Windows 2008.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 415574 [details] [diff] [review]
v1 for 3.5.2+

No, the right solution is to use Win32::Symlink, if it still works.

Comment 5

[Michael Thomas (Mockodin)]

It needs to be updated before it could be considered. In theory is partially works. But uses outmoded methods. MKLink didn't exist at the time of its last update which is 2004.

I think another option is to ping ActivePerl to compile with symlink support natively which should now be possible... It would just need to check for Os version.. which incidentally I planned for in the patch :-)

Comment 6

[Michael Thomas (Mockodin)]

Win32 stat and filetests unaware of new-to-Vista NTFS symlinks
  http://bugs.activestate.com/show_bug.cgi?id=81273

symlink on windows vista and above should be enabled
  http://bugs.activestate.com/show_bug.cgi?id=85472

Comment 7

[Max Kanat-Alexander]

(In reply to comment #5)
> It needs to be updated before it could be considered. In theory is partially
> works. But uses outmoded methods. MKLink didn't exist at the time of its last
> update which is 2004.

  Okay. Do the outmoded methods have any functionality difference in terms of the symlink? And do they work on older versions of Windows?

> I think another option is to ping ActivePerl to compile with symlink support
> natively which should now be possible... It would just need to check for Os
> version.. which incidentally I planned for in the patch :-)

  You could request it. I'd say go ahead and file a bug in their Bugzilla if there isn't already one.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #6)
> Win32 stat and filetests unaware of new-to-Vista NTFS symlinks
>   http://bugs.activestate.com/show_bug.cgi?id=81273
> 
> symlink on windows vista and above should be enabled
>   http://bugs.activestate.com/show_bug.cgi?id=85472

  Ah, thanks for filing those! :-)

Comment 9

[Michael Thomas (Mockodin)]

(In reply to comment #7)
>   Okay. Do the outmoded methods have any functionality difference in terms of
> the symlink? And do they work on older versions of Windows?

useful info: http://en.wikipedia.org/wiki/NTFS_symbolic_link

The old method was hard links and directory junctions which is what I would assume they tie into since real symlinks (or as real as they are now) didn't exist previously, so.. I certainly wouldn't use it on my server even if it does work on older versions.

Comment 10

[Michael Thomas (Mockodin)]

Another good post:
http://www.nntp.perl.org/group/perl.module-authors/2009/01/msg7177.html

Comment 11

[Max Kanat-Alexander]

Thanks for the references.

In our particular case an NTFS junction is absolutely fine. If somebody deletes the directory, it's not a problem at all--just checksetup.pl does that, and it does it because it's clearing out the whole parent directory anyhow. The directories will never be on separate volumes (since we're linking within the directory itself), and we're always linking directories.

Until Win32::Symlink (or the built-in "symlink" in ActiveState) supports the modern symlink methods, I don't see a problem with using junctions.

Comment 12

[Michael Thomas (Mockodin)]

Attachment: v2

Reply from ActiveState lead me to this.

use Win32::API;
Win32::API->Import('kernel32', "CreateSymbolicLink",['P','P','N'],'I');
CreateSymbolicLink($symblink, $target, $IsDir);

Comment 13

[Michael Thomas (Mockodin)]

Attachment: v3

Mkanat brought up FAT32 Support, since Symbolic Links only work on NTFS we need to check for that in addition to the OS version.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 429279 [details] [diff] [review]
v3

  Okay, architecturally, I'd like to see all the code inside of _do_template_symlink. You can return from the method if the OS isn't qualified to do it.

  My other comments inline below:

>+    if (ON_WINDOWS && Win32::FsType() =~ m/^NTFS/i){

  What is FsType checking? It's the file system that templatedir is on that we care about, of course, not the OS filesystem, so I just want to make sure that we're checking the FS that the templatedir is on.

>+	# Windows Vista and above support a symlink command
>+	# Must be OS major version 6 or above
>+	my @OS = Win32::GetOSVersion();
>+        shift @OS;
>+        my ($major, $minor, $build) = @OS;
>+        if ($major >= 6){
>+	    $windows_symlink_ok = 1;
>+	}

  Spacing, there.

>     my $abs_root  = dirname($abs_path);
>+    # If we are on Windows we need to fix some brokenness
>+    if (ON_WINDOWS){
>+        $abs_root =~ s/://;
>+        $abs_root = "/$abs_root";

  Could you explain that some more? Perhaps a better solution would be for us to use File::Spec?

>@@ -886,8 +905,16 @@
>         my $relative_target = File::Spec->abs2rel($target, $container);
> 
>         my $link_name = "$container/$dir_name";
>-        symlink($relative_target, $link_name)
>-          or warn "Could not make $link_name a symlink to $relative_target: $!";
>+        if (!ON_WINDOWS){

  It's easier to read code if booleans are positive. So put the ON_WINDOWS code here, and the non-Windows code in the block below.

>+            symlink($relative_target, $link_name)
>+              or warn "Could not make $link_name a symlink to $relative_target: $!";
>+        } else {
>+            my $createsymboliclink = Win32::API->new('kernel32', "CreateSymbolicLink",['P','P','N'],'I');

  Could you add a comment here explaining what all those arguments mean, just really briefly. Like # path, path, number, returns integer

>+            $createsymboliclink->Call($link_name,$relative_target, 1 )
>+             or warn "Could not make $link_name a symlink to $relative_target: $!";

  Nit: Spacing.

  Perhaps the actual symlink creation should be moved into another function, so that the warning only has to be written once.

Comment 15

[Michael Thomas (Mockodin)]

Attachment: v4

Split out NTFS check, symlink and windows os version check for windows in to separate functions.

Would we want any or all of these moved to Util.pm?

Comment 16

[Max Kanat-Alexander]

Comment on attachment 429541 [details] [diff] [review]
v4

>+    # If we are on Windows we need to fix some brokenness
>+    # Other wise it tries to create a path with a : in the middle of it
>+    # We will also need to prepend a slash under windows.

  Like, it includes the C: in there? I think File::Spec should probably be used to parse out the parts, no?

>+	# Windows requires NTFS and a OS version higher than 6
>+	# Well test the folder containing the Symblink to be created
>+	# and test the target
>+	if (ON_WINDOWS && _get_windows_os_major_version() >= 6){
>+	    return unless _windows_ntfs_check($container) && _windows_ntfs_check($relative_target);
>+       }

  This check should be inside of create_symb_link, no? Hmm, I do see the problem with the return value. Perhaps the warning should be somehow moved into _create_symb_link.

>+# Symlink creation at present symlink is not compiled in the Perl Core
>+# for the Windows OS so we provide a workaround.
>+sub _create_symb_link {

  Let's call it _create_symlink. Or, we could move it to Bugzilla::Install::Util, and call it create_symlink. Up to you.

>+    # Return Value:
> [snip]

  Awesome comment! :-)

>+    # At Present SymLink is not support in the Perl Core for Windows
>+    # However Symlink support was added to Windows as of Windows Vista

  Okay. So this doesn't do NTFS Junctions for older releases?

>+# NTFS Check for Windows
>+sub _windows_ntfs_check {
>+    # We will need to record the current directory so we can switch
>+    # back after our test, so we record it here.
>+    use Cwd;
>+    my $currentdir = getcwd;

  Currently this isn't actually necessary, because we have to be in the Bugzilla directory in order for checksetup.pl to run. If we aren't running checksetup.pl, this won't work, because cwd() is tainted.

Comment 17

[Michael Thomas (Mockodin)]

Attachment: v5

Posting what I think is a simpler patch.

I took a look at Win32::Symlink which implements junctions for Windows 2003 R2 and before. In that module they actually override CORE::GLOBAL::symlink. So I have adapted the concept here and allowed for older systems where junctions were the only option.

Result: Support for Junctions and Symlinks without having to call them special for every use.

symlink() will now work anywhere so long as Util.pm has been used at least once.

Comment 18

[Michael Thomas (Mockodin)]

Oh and File::Spec I didn't see anything there that would address this.

The Issue is:
$abs_root might equal 'c:/bugzilla/'

We then concat it with '$datadir/template'

> my $container = "$datadir/template$abs_root";

This would result in:
"c:/bugzilla/data/templatec:/bugzilla/";

which is invalid, by removing the : and prepending the / we get

c:/bugzilla/data/template/c/bugzilla/

which is valid

Comment 19

[Max Kanat-Alexander]

  Ah, thanks for the explanation about File::Spec. Could you explain that in the comment?

  Also, did you want review on this patch?

Comment 20

[Michael Thomas (Mockodin)]

Bah, if I'm not requesting from the wind, I'm forgetting to request period.
I'll update the patch with comment 18 info then will request review

Comment 21

[Michael Thomas (Mockodin)]

Attachment: v6

updated description for abs_root fix for windows

Comment 22

[Michael Thomas (Mockodin)]

Attachment: v6.1

Noticed the requirements module had the check reversed, should only ask for Win32::SymLink if OS version < 6 (before vista)

Comment 23

[Max Kanat-Alexander]

Comment on attachment 429913 [details] [diff] [review]
v6.1

>Index: MSSQL_BUGZILLA/Bugzilla/Template.pm
>+        # Junctions and Symlinks on Windows require a NTFS filesystem
>+	return unless Win32::FsType() =~ m/NTFS/i;

  I think that should just be inside the symlink implementation, and checksetup can rightfully throw a warning if we can't do it.

>Index: MSSQL_BUGZILLA/Bugzilla/Util.pm
>@@ -627,6 +628,43 @@
>     }
> }
> 
>+sub windows_os_major_version {

  This can't go into Bugzilla::Util, because Bugzilla::Util can't be reliably used at  installation time. It has to go into Bugzilla::Install::Util.

>+# Turns on symlink support for Windows Vista and newer
>+*CORE::GLOBAL::symlink = sub {

  Instead of this, you can just create an export a "symlink" subroutine that has the same prototype as the built-in "symlink" function. You shouldn't change the actual value of CORE::GLOBAL, because that's how people access the native symlink function.

>+    use Win32::API;

  There should never be a "use" inside of a subroutine, because they always happen at compile-time. This should remain a "use if" like you had it in the last patch.

>+} if ON_WINDOWS
>+      && windows_os_major_version() >= 6;
>+
>+# Turns on symlink support for Windows 2003 r2 and older
>+eval 'Win32::SymLink' if ON_WINDOWS && windows_os_major_version() < 6;

  You left the "use" out of the eval.

  What I'd rather see is a "symlink" function that calls either the Win32::Symlink function, the Win32::API kernel call, or CORE::GLOBAL::symlink. I think that might be simpler as far as code-reading goes. 

>Index: MSSQL_BUGZILLA/Bugzilla/Install/Requirements.pm
>+    if (ON_WINDOWS) {
>+        my @OS = Win32::GetOSVersion();
>+        my ($string, $major, $minor, $build) = @OS;
>+        if ($major < 6) {

  If you put the OS version stuff into Install::Util, you can use it here.

Comment 24

[Michael Thomas (Mockodin)]

Trying to test Win32::Symlink to see if it compiles under 5.10, shows up as UNKNOWN on cpan. The test notes being not encouraging. Also investigating the possibility of rebuilding the module or incorporating what we need directly.

Comment 25

[Michael Thomas (Mockodin)]

Attachment: v7

Barring more effort than I have time for atm, Win32::Symlink is broken at least without going to a fair about of effort my few passes at building failed.. I did not try overly hard however.

Reattached a slightly updated patch for Vista and higher.

Comment 26

[Max Kanat-Alexander]

Comment on attachment 436378 [details] [diff] [review]
v7

  Instead of overriding CORE::GLOBAL::symlink (you're not supposed to modify CORE::GLOBAL functions), just create a "symlink" function that Bugzilla::Util will export.

Comment 27

[Michael Thomas (Mockodin)]

Attachment: v7

Corrected Upload, uploaded incorrect version previously.

Comment 28

[Max Kanat-Alexander]

Comment on attachment 436378 [details] [diff] [review]
v7

>=== modified file Bugzilla/Install/Util.pm
>+if (ON_WINDOWS  && require Win32::API){

  Also, you should just do this require inside of the if.

Comment 29

[Max Kanat-Alexander]

Comment on attachment 436380 [details] [diff] [review]
v7

Same comments.

Comment 30

[Michael Thomas (Mockodin)]

(In reply to comment #28)
> (From update of attachment 436378 [details] [diff] [review])
> >=== modified file Bugzilla/Install/Util.pm
> >+if (ON_WINDOWS  && require Win32::API){
> 
>   Also, you should just do this require inside of the if.

  Also, you should just do this require inside of the if.

What if? It's in an if

(In reply to comment #26)
> (From update of attachment 436378 [details] [diff] [review])
>   Instead of overriding CORE::GLOBAL::symlink (you're not supposed to modify
> CORE::GLOBAL functions), just create a "symlink" function that Bugzilla::Util
> will export.

IRC a few moons ago, you OK'd the *CORE::GLOBAL::symlink
http://bugzilla.glob.com.au/irc/?a=date&s=3+Mar+2010&e=3+Mar+2010&h=native+symlink+function

That said at the time I still had hope for Win32::Symlink for pre-vista support.

Comment 31

[Max Kanat-Alexander]

>In reply to comment #30)
> > >+if (ON_WINDOWS  && require Win32::API){
> > 
> >   Also, you should just do this require inside of the if.
> 
>   Also, you should just do this require inside of the if.
> 
> What if? It's in an if

  if (ON_WINDOWS) { require Win32::API;

  I'm pretty sure Win32::API is always required by us on Windows, isn't it?

> (In reply to comment #26)
> > (From update of attachment 436378 [details] [diff] [review] [details])
> >   Instead of overriding CORE::GLOBAL::symlink (you're not supposed to modify
> > CORE::GLOBAL functions), just create a "symlink" function that Bugzilla::Util
> > will export.
> 
> IRC a few moons ago, you OK'd the *CORE::GLOBAL::symlink
> http://bugzilla.glob.com.au/irc/?a=date&s=3+Mar+2010&e=3+Mar+2010&h=native+symlink+function
> 
> That said at the time I still had hope for Win32::Symlink for pre-vista
> support.

  Okay. Well, I'd rather export a "symlink" subroutine if it doesn't throw an error.

Comment 32

[Michael Thomas (Mockodin)]

(In reply to comment #31)

>   I'm pretty sure Win32::API is always required by us on Windows, isn't it?
We didn't but you recently added sub prevent_windows_dialog_boxes which incidentally uses the same method for loading I use, except your wrapping in an eval, is an eval necessary when inside a if clause? Didn't think it was, not that it hurts anything.

In any case, no, unless we move that require outside of both subs.

>   Okay. Well, I'd rather export a "symlink" subroutine if it doesn't throw an
> error.

Ok that's exactly what I did, only I made the "export" override the non-existent core function.

I think attachment 436378 [details] [diff] [review] is good to go as is.

Comment 33

[Frédéric Buclin]

IMO too risky for 4.0 and 4.2.

Comment 34

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.