Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Example output folders #73

Open
andersmelander opened this issue Apr 18, 2019 · 9 comments

Comments

4 participants
@andersmelander
Copy link
Member

commented Apr 18, 2019

The current examples are all configured as follows:

  • Output directory: ..\..\..\Binaries\$(Platform)\$(Config)
  • Search path: ..\..\..\Unit Cache;..\..\..\Source
  • Unit output directory: ..\..\..\Unit Cache\$(Platform)\$(Config)

I propose that this be changed to:

  • Output directory: .\bin
  • Search path: ..\..\..\Source
  • Unit output directory: .\lib

or if we must have the platform/config part, of which I'm not a fan, then:

  • Output directory: .\bin\$(Platform)\$(Config)
  • Search path: ..\..\..\Source
  • Unit output directory: .\lib\$(Platform)\$(Config)

The important part is that the individual examples should not have the unit output folder of other examples in their search path.
One reason is that many examples uses the same unit names (for example MainUnit.pas) and the compiler isn't always smart enough to discover that the mainunit.dcu in the search path doesn't correspond to the mainunit.pas in the current project. The result is that the compile fails if one is lucky or that the application bombs spectacularly at run time if unlucky. In any case it's a bad practice.
The reason I have made the bin and lib folders subdirectories of the project folder is that it makes them easier to find and the examples more self contained. The choice of bin and lib is just personal taste; It could just as well be binaries and library or whatever.

@AngusJohnson

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

I propose that this be changed to:

  • Output directory: .\bin
  • Search path: ......\Source
  • Unit output directory: .\lib

This would be my preference too.

@micha137

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

I'm a fan of $(Platform)\$(Config), since without it when switching between platforms and configs (especially 32 und 64 bit), the .dcu files get into each others' way. But nevertheless, I would write a script to adjust the paths as proposed (without $(Platform)\$(Config)), so switching to one or the other style is easy.

@micha137 micha137 self-assigned this May 2, 2019

@andersmelander

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

@micha137 Yes, I understand that in a real project, which needs to support multiple platforms or build configs, one would use $(Platform)\$(Config) or something like it, but in this case we are just talking about the examples.

Do we really need to handle that someone might compile the examples for both 32 and 64 bit? I'm not saying that it's impossible I just think it would be very rare - and it can be handled just by doing a Build instead of a Compile. Those that compile for 64 bit probably already know that. Anyhow I don't feel strongly about this.

I see that you assigned yourself to this task. You should coordinate with @AngusJohnson since he has already done some work on this and I believe he has a solution ready.

@micha137

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Yes, no problem to switch by rebuilding.

I hope @AngusJohnson has not changed this yet, as my 42 line script is done, fixing *dproj und *.cbproj, but not *.lpi.

@AngusJohnson

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Hi Micha. I'm very happy if you want to manage this. Go for it :).

micha137 added a commit that referenced this issue May 2, 2019

Fix example search and output paths
* for our 39 Delphi Examples and for our 5 C++ Builder examples
* leaving the Lazarus examples as they are (but the *.lpi files don't use a common unit cache directory anyway and use a UnitOutputDirectory of "lib/$(TargetCPU)-$(TargetOS)")
* see #73
@micha137

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Should also put the script for fixing the paths somewhere, like a folder maintenance? I have two more maintenance scripts: dprojgenerator.pl and delphinus-consistencycheck.pl.

@CWBudde

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Sorry to comment this so late, but as I mentioned before, I had a bit of work to do for my current project I earn my money with.

Regarding this issue: I might be the only one who have objections with it, but here we go:

  1. Output directory: .\bin
    It have picked "Binaries" because we also have "Source" and not "src". Note also the capital "B" as in "Examples".

Actually the name shouldn't matter, but for sake of consistency I would vote to keep it as it is.

When I started working in the GR32 team, the members were very strict when it came to naming. The code should be formatted according to the Pascal Style Guide. Within the guide procedure and field names started with a capital letter. Thus I would prefer to keep the capital letter here:

  1. Unit output directory: .\lib
    There are two problems I see here. First again the abbreviation vs. the full name "Library". But second, DCUs are not library files. They are precompiled units, which - technically spoken - differ from libraries. Thus using the name 'lib' is misleading.

  2. Search path: ......\Source
    Having the precompiled path in the search path does make sense when you quickly want to compile all examples. You only need to compile the basic library once. With this precompiled the rest is blazingly fast. This is especially important if you want to see if something is broken or if you want to test on both win32 and win64 quickly.

For this reason I'd also opt for using $(Platform)$(Config)

The only problem we have is the fact that there are currently many example units named "MainUnit". It would be necessary to rename them in order to see a benefit.

micha137 added a commit that referenced this issue May 2, 2019

maintenance scripts
* to be run with Cygwin Perl
* they will need minor adjustments to get the paths right, since I moved these scripts into the subdirectory "Maintenance" now
* see #73
@andersmelander

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Binaries, Capital letters.

Sure. No problem.

Lib

Habit from my days as a C/C++ developer where intermediaries would go into lib. I guess the more correct term would be Obj for "object files". Under any circumstance the folder is an implementation detail that is not important to the purpose of the examples: To demonstrate how graphics32 works.

Having the precompiled path in the search path does make sense when you quickly want to compile all examples.

Speed surely can't be the overriding reason for this decision. It's Delphi we're talking about; I don't think it matters if it takes 1 second or 1.2 to compile an example.

This is especially important if you want to see if something is broken

I understand your point but isn't the target audience of the examples the users of graphics32? In any case my experience is that compile speed is a non-problem with the examples.

@micha137

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@CWBudde: Sorry I went forward already, but with the script it is (relatively) easy to change again.

Capitalisation for consistency is good, not abbreviating is also fine with me.

I just checked why we didn't hit the cache directory issue before, considering MainUnit.pas is used in Examples/Blending/PixelCombine since at least 2012, and having a main unit in every example is a matter of consistency. Commit b0f751a (from 25th Mar 2019) introduced ..\..\..\Unit Cache. I don't see other solutions than not having a cache or not having two equally named units. I'm fine with either, as long as the examples build out of the box in my Jenkins.

I also have to add $(crossvcl)\Lib\$(ProductVersion)\$(Platform)\$(Config) back in, because otherwise CrossVCL support is broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.