Recent

Author Topic: Issues publishing a project (No subdirectories incl., no non-Pas. files incl.?)  (Read 21182 times)

JuhaManninen

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 4459
  • I like bugs.
I implemented the remaining functionalities(r. 58851):
Ah yes, you had the full commit rights already.
I cleaned some parts in r58853. Project and Package options have a common ancestor TBaseCompilerOptions which I forgot last time. The typecast for TProject() was wrong with packages. BackupDir and UnitOutputDir were calculated repeatedly every time a file was found which is not wise.
I also cleaned the ToDos etc. in TPublishProjectOptions.

It works but to me it looks overly complex.
Quote
2. Update relative paths in the published lpi
Why is this needed? Is it only for the weird directory structures spreading over c:\ and d:\ drives? I don't think we should even support such weirdness.
Otherwise paths in .lpi should not need adjustment because the directory structure is always kept. The file references are always relative AFAIK.

FDestProjFiles seems redundant. It is filled by iterating project files: "for i:=0 to CurProject.UnitCount-1 do"
Why not iterate them again when needed? It is used for updating paths in .lpi which also may be useless.

Why is another FindAllFiles() needed for the zipper? We already know which files were copied. BTW, FindAllFiles() uses the same TFileSearcher class that we already use.

Why all the UpperCase() calls? The code in Lazarus dealing with units, projects and packages does not UpperCase everything yet it works. What happens if you remove them?

I think TProject.WriteProject() is buggy, too. It fails to write all build modes when I have 3 of them (Default, Debug and Release). That is not caused by the Publish Module code of course.
Mostly Lazarus trunk and FPC 3.2 on Manjaro Linux 64-bit.

balazsszekely

  • Guest
Quote
Why is this needed? The file references are always relative AFAIK.
Unfortunately no, not on windows at least. If you use the "Add files from the filesystem" and the file is not in the current directory, Project Inspector will add the full path of the file(see attached image). You don't have to take to the extreme, like spreading the files to c: and d: drives.

Quote
Otherwise paths in .lpi should not need adjustment because the directory structure is always kept.
Since the lpi contains absolute paths, like the one in the screenshot, taking the published folder to another computer where the particular file does not exists, will lead to load/build failure. This is why I update the lpi file. Every path becomes relative to the published folder, more precisely relative to the lpi file inside the published folder. Under Linux/GTK2(just tested) apparently the paths are always relative, which is good.

Quote
I don't think we should even support such weirdness.
In my opinion we should support it. As the current thread already showed, people do weird stuff, even if it's not recommended. Ideally no matter how spread the project is, the publish feature should bring it to one common folder, preserving the directory structure, but I don't want to insist on this subject, I can easily remove the support if necessary.

Quote
FDestProjFiles seems redundant. It is filled by iterating project files: "for i:=0 to CurProject.UnitCount-1 do"
Why not iterate them again when needed? It is used for updating paths in .lpi which also may be useless
FDestProjFiles contains the destination files, the one in the published folder, CurProject.UnitCount refers to the actual units. Again this is only needed when absolute path are used.

Quote
Why is another FindAllFiles() needed for the zipper? We already know which files were copied. BTW, FindAllFiles() uses the same TFileSearcher class that we already use.
OK, this one is indeed redundant. FDestProjFiles can be used to construct the zip file.

Quote
Why all the UpperCase() calls? The code in Lazarus dealing with units, projects and packages does not UpperCase everything yet it works. What happens if you remove them?
Call me paranoid, but I always compare strings with upper/lower case. Most likely will work without uppercase too.

JuhaManninen

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 4459
  • I like bugs.
Quote
Why is this needed? The file references are always relative AFAIK.
Unfortunately no, not on windows at least. If you use the "Add files from the filesystem" and the file is not in the current directory, Project Inspector will add the full path of the file(see attached image).
That is a bug. This is a cross-platform system, projects and packages should be portable between computers and OSs even without the publish feature.
How long has it been like that? Does it happen only with the top level directories?
I should test more on Windows. Linux just works so well for me that I wouldn't want to...

Quote
OK, this one is indeed redundant. FDestProjFiles can be used to construct the zip file.
+ the filtered files. FCopiedFiles has them all, if you adjust the path.

Quote
Call me paranoid, but I always compare strings with upper/lower case. Most likely will work without uppercase too.
We should not have useless calls. It makes the code more difficult to follow and understand. I can test without UpperCases later.
Mostly Lazarus trunk and FPC 3.2 on Manjaro Linux 64-bit.

JuhaManninen

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 4459
  • I like bugs.
I tested on Windows. Files in a project are saved with absolute paths only if the path reference goes up to the root, meaning the common parent path is eg. C:\ or D:\ root.
Is there a technical reason why ExtractRelativePath() or similar functions cannot handle it?
The bug should be fixed where it actually is, not in the Publish Module code.
If you want to support projects split between C:\ and D:\ in the Publish Module then let's do it explicitly for only that case. When the drive letter differs from the main dir's drive then copy those files to main dir.
On a Unix based systems there is no such issue because all directories are under the same tree, even if they are on different physical hard drives.

I tested more and edited the .lpi file manually, replacing the absolute path with relative. Then I reopened the project. The file was found correctly but its path shows as absolute in Project Inspector! So the relative path is understood and it works but somewhere it is wrongly converted to absolute. It can happen in utility functions of either FPC or Lazarus, who knows.
Can you please debug where the error happens.
« Last Edit: September 04, 2018, 05:54:18 pm by JuhaManninen »
Mostly Lazarus trunk and FPC 3.2 on Manjaro Linux 64-bit.

balazsszekely

  • Guest
@Juha
In my opinion we are wasting to much time on a feature that apparently only we are interested.  :)
I did the following:
1. Removed UpperCase
2. Removed the update of the lpi file
3. Removed FDestProjFiles(I'm not sure about this, the code looks better, but since I have to do a lot of stringreplace to get the destination files, it's much slower)
4. Other minor fixes

As a conclusion the Publish project feature only fails in extreme cases. Fixing the absolute path(windows only) issue, will improve the feature even more. I attach the files from r. 58851 as future reference, if somebody wish to handle extreme cases on windows.

JuhaManninen

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 4459
  • I like bugs.
In my opinion we are wasting to much time on a feature that apparently only we are interested.  :)
No, I believe it will be a very popular feature once it works well. For some reason nobody else has commented about it yet ...
Thanks for your changes. Actual bugs are easier to spot and fix when there is no extra code around.
I did minor changes in r58870 and improved the debug output.

Quote
3. Removed FDestProjFiles(I'm not sure about this, the code looks better, but since I have to do a lot of stringreplace to get the destination files, it's much slower)
StringReplace is used in 2 places with identical code :
Code: Pascal  [Select][+][-]
  1. Drive := ExtractFileDrive(RelPath);
  2. if Trim(Drive) <> '' then
  3.   RelPath := StringReplace(RelPath, AppendPathDelim(Drive), '', [rfIgnoreCase]);
Drive is always found at the beginning of RelPath, isn't it? Why not use Delete() with index 1?
Code: Pascal  [Select][+][-]
  1. Delete(RelPath, 1, Length(Drive));
or similar. I think trimming the Drive is useless. All paths have been trimmed at this point.

Quote
As a conclusion the Publish project feature only fails in extreme cases. Fixing the absolute path(windows only) issue, will improve the feature even more. I attach the files from r. 58851 as future reference, if somebody wish to handle extreme cases on windows.
r.58851 can also be found in revision control easily. There is no reason to avoid revision control tools. They are such a good invention.
Everybody please test. There is comprehensive debug output by DebugLn() calls. It will be saved to a file when you start Lazarus with parameter --debug-log=<file>.
 http://wiki.freepascal.org/LazLogger#File
« Last Edit: September 05, 2018, 08:30:16 pm by JuhaManninen »
Mostly Lazarus trunk and FPC 3.2 on Manjaro Linux 64-bit.

howardpc

  • Hero Member
  • *****
  • Posts: 4144
Sorry, I seem to find a bug the first time I tried the trunk version.
I selected the

$(ProjPath)/published/

option for the destination directory (no filter and no compression) and immediately the attached error dialog was shown.
I did not create the /published subdirectory, assuming the tool would do that.
« Last Edit: September 05, 2018, 06:50:57 pm by howardpc »

JuhaManninen

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 4459
  • I like bugs.
Ah, I thought I was clever when I added the extra check for a subdirectory.
It must be allowed, yes. Wait...
... Now it creates a recursive loop. I will think how to fix it. Maybe it must be disabled after all.

Ok, please test with r58874.
I disallowed publishing into a subdirectory of the project/package, removed the default path selection for it and improved the error messages.
I am not sure how easy it is to prevent the recursive loop otherwise.
I don't see this as a big restriction. Any other writable directory will do.

If somebody wants to test the subdirectory stuff, just define AllowProjectSubDirectory.
There is now line:
{.$define AllowProjectSubDirectory}
Remove the dot and build Lazarus.
« Last Edit: September 05, 2018, 08:42:27 pm by JuhaManninen »
Mostly Lazarus trunk and FPC 3.2 on Manjaro Linux 64-bit.

balazsszekely

  • Guest
@howardpc
Thanks for testing. Finally we have some feedback. Much much appreciated.

@Juha
Quote
Now it creates a recursive loop. I will think how to fix it. Maybe it must be disabled after all.
There is no recursive loop. Initially the project files are copied in the published folder, which is a subfolder in this particular case of the source directory. Then when IDE search for extra files will find the already copied project files(filter match *.pas files), then fails to copy them again since the source and the destination are the same. The FCopyFailedCount gets bigger then zero and the Run method exits without giving any clear feedback to the user. We need to add more message dialogs. Here is the antidote:
Code: Pascal  [Select][+][-]
  1. procedure TPublisher.DoFileFound;
  2. // Copy a found file if it passes the filter.
  3. var
  4.   CurDir: string;
  5. begin
  6.   //....
  7.   if (CurDir = FDestDir) then <--This one
  8.   begin
  9.     DebugLn(['DoFileFound: The destination directoty is in the same folder as the source files, skipping files: ', FileName]);
  10.     Exit;
  11.   end;            
  12.   if FOptions.FileCanBePublished(FileName) then
  13.   begin
  14.     if CopyAFile(FileName) <> mrOK then
  15.       Inc(FCopyFailedCount);
  16.   end
  17.   else
  18.     DebugLn(['DoFileFound: Rejected file ', FileName]);
  19. end;

Ps: The
Code: Pascal  [Select][+][-]
  1. if FCopiedFiles.IndexOf(AFilename) >= 0 then exit;
fails because the files inside the published folder are not added to stringlist.
« Last Edit: September 05, 2018, 08:49:11 pm by GetMem »

lucamar

  • Hero Member
  • *****
  • Posts: 4219
It must be allowed, yes. Wait...
... Now it creates a recursive loop. I will think how to fix it. Maybe it must be disabled after all.

How about checking if destination is under origin and, if so, excluding it from any further origin file search? Just gave the source a quick glance but it doesn't seem too difficult to implement.
Turbo Pascal 3 CP/M - Amstrad PCW 8256 (512 KB !!!) :P
Lazarus/FPC 2.0.8/3.0.4 & 2.0.12/3.2.0 - 32/64 bits on:
(K|L|X)Ubuntu 12..18, Windows XP, 7, 10 and various DOSes.

JuhaManninen

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 4459
  • I like bugs.
Sorry guys, I edited my post before noticing the replies.
Thanks for testing.
Yes I believe the subdir problem can be solved. Now however I keep a small pause from this publish thingy and do other things. Maybe some of you will find a clean solution for it.
Mostly Lazarus trunk and FPC 3.2 on Manjaro Linux 64-bit.

balazsszekely

  • Guest
Quote
Yes I believe the subdir problem can be solved. Now however I keep a small pause from this publish thingy and do other things. Maybe some of you will find a clean solution for it.
OK. Fixed in r.58876. Publishing to a subfolder is still possible. Please test. Thank you.

JuhaManninen

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 4459
  • I like bugs.
OK. Fixed in r.58876. Publishing to a subfolder is still possible. Please test. Thank you.
No it is not possible. The same recursive loop still happens. The "Published" directory is created under the project dir which then contains the project + another "Published" directory which then contains the project + another ... and so on until the OS is out of memory and kills Lazarus.
I attach the project I am testing with. It (obviously) has source files above the project's main dir.
It is (obviously) zipped using this new great publish feature, although not to a project's subdirectory. :)
Mostly Lazarus trunk and FPC 3.2 on Manjaro Linux 64-bit.

howardpc

  • Hero Member
  • *****
  • Posts: 4144
For me, R58877 publishing to a /published subdirectory works well, whether compressed or not.
Does just what it says on the tin.

balazsszekely

  • Guest
@Juha
Quote
No it is not possible. The same recursive loop still happens. The "Published" directory is created under the project dir which then contains the project + another "Published" directory which then contains the project + another ... and so on until the OS is out of memory and kills Lazarus.
I attach the project I am testing with. It (obviously) has source files above the project's main dir.
Please try one more thing, add the following two line just below Result := mrOk(TPublisher.CopyAFile).
Code: Pascal  [Select][+][-]
  1. function TPublisher.CopyAFile(const AFileName: string): TModalResult;
  2. var
  3.   RelPath, LfmFile: string;
  4.   Drive: String;
  5. begin
  6.   Result := mrOK;
  7.   if Pos(FDestDir, ExtractFilePath(AFileName)) > 0 then
  8.     Exit;  
  9.   //....
  10. end;
This will prevent any recursive copy.

Quote
It is (obviously) zipped using this new great publish feature, although not to a project's subdirectory. :)
:) It was harder to implement then I initially thought.



@howardpc
Quote
For me, R58877 publishing to a /published subdirectory works well, whether compressed or not.
Does just what it says on the tin.
Thank you. Hopefully the publish project feature will become bug free soon.

 

TinyPortal © 2005-2018