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.

Sign up for GitHub

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

Jump to bottom

Incorrect references and dependencies #4270

Merged
merged 1 commit into from
Jan 30, 2018
Merged

Incorrect references and dependencies #4270

merged 1 commit into from
Jan 30, 2018

Conversation

gerhardol
Copy link
Member

Corrects #4269

GitExtensions (cs project) has References to the plugin .dll which should not occur (no direct access to the .dll)
Build dependencies do not include all subprojects. So for instance when debugging, a clean was required first (partly hidden by the previous issue).

How did I test this code:

  • Clean the solution.
  • Verify that .dlls are removed. GitExtUtils.dll is still remains though.
  • Start debugging GitExtensions
  • Verify that the AppVeyor plugin is loaded

Has been tested on (remove any that don't apply):

  • Windows 10

@RussKie
Copy link
Member

RussKie commented Dec 23, 2017 via email

@gerhardol
Copy link
Member Author

gerhardol commented Dec 23, 2017 •
edited

Right click on the GitExtensions project and select Build Dependencies.
Previously, for instance AppVeyor was listed after GitExtensions, so it was not built when debugging.
A full build would have generated the .dll, but if you make a change to the plugin, you have to force rebuild it again.
For the BitBucket plugin that was listed as a Reference, it was enough to clean the project before debugging.

Note: All projects may not need to be added to the GitExtensions (project). The plugins are copied in target AddPluginBinaries so they are for sure needed, but some of the other may be handled indirectly (like if SpellChecker and ResourceManager that likely are used by other dependent projects too). Easier to select all (but tests, VS plugin and Setup).

Note2: No check for Mono.

@RussKie
Copy link
Member

RussKie commented Dec 23, 2017 via email

@RussKie
Copy link
Member

RussKie commented Dec 24, 2017

Previously, for instance AppVeyor was listed after GitExtensions

I can only guess that the reason all projects are referenced from GitExtensions project is to ensure they all get built before it.
This may not be necessary, as per your suggestion, and can be achieved by setting the build order. This is, however, manual work and can get out-dated...

This is what copies all plugins to the target directory https://github.com/gitextensions/gitextensions/blob/master/TranslationApp/TranslationApp.csproj#L167-L185
This is why TranslationApp is built last.

@gerhardol
Copy link
Member Author

This is what copies all plugins to the target directory

The same when building GitExtensions.csproj

For debug to work properly the build system must understand that it should build all plugins first. This can be done in several ways (not mutually exclusive or complete list).

  • Depending on a project that collects all plugins, possibly with checks (this how I would have implemented this with GNU make or CMake or gradle, not so easy with MSBuild).
  • Use manual dependencies in the project so all plugins are built before the .exe (what this PR proposes)
  • Use manual project references as was done for some plugins. Not recommended as the plugin code can be used directly by the application.
  • Let the build system figure out the dependencies. I cannot see how this can be done here. Just because a plugin has references to the application, it does not mean that it has too be included.

The references were not really a problem, just done the wrong way.

The TranslatorApp is not built last right now. This PR pushes the build of the translator app later, but some plugins as well as GitExtensions and the Setup are built after TranslatorApp. Only tests should be after. (The translator app do not need dependencies to all plugins as long as they are referenced by the main app). Without knowing the details how the translator app should work, I assume this and will push an update.

@gerhardol
Copy link
Member Author

I ran the TranslationApp again after the update.
The Bitbucket Server binaries were renamed in #4210 an the app seem to have run where the internal .dll was not cleaned, so bitbucket resources should be duplicated on the server (I just get Load error when trying to view).

Also missing (should not be translated?)

SHA1

@RussKie
Copy link
Member

RussKie commented Dec 25, 2017 •
edited

In ideal world plugins would live in own/separate repo(s) and get installed/loaded by the user. We had some high level discussions before but nothing of substance.
Such move comes with a lot of overhead, e.g. we will probably need to build some sort of plugin store, delivery mechanisms, compatibility checks etc....

I don't think any of the methods mentioned above address the issue of renamed assemblies in the target directory.
As I said, I don't have the issue you're facing, but I am happy to consider alternatives if they make DevEx easier.

@gerhardol
Copy link
Member Author

I would suggest that the Setup project is removed from GitExtensions.VS2015.sln.
The msi package is built from BuildInstallers.VS2015.cmd (that calls MakeInstallers.cmd) to build the shell extension different targets and SshAskPass. The last are C++ projects and not included in the general solution, the setup project could be enhanced to handle this I assume, but for now it is clearer to remove .wixproj from GitExtensions.VS2015.sln.

@RussKie
Copy link
Member

RussKie commented Dec 25, 2017 via email

@gerhardol gerhardol mentioned this pull request Jan 27, 2018
Open
@RussKie
Copy link
Member

RussKie commented Jan 29, 2018

I've been getting dramas with the dependencies lately - not all files are getting copied into Debug/bin folder and then Fusion is complaining...
So, if this helps, it will be a ripper!
I will test it ASAPP

@gerhardol
Copy link
Member Author

I suggest I rebase and make some adjustments first.
Both due to the projects have changed and that due to I have tuned some more locally

I have done some other changes to the build and dependencies too as well as 201572017 unification, that could go in later

@gerhardol
Incorrect references and dependencies
644cfc9 
Corrects gitextensions#4269

    GitExtensions (cs project) has References to the plugin .dll which should not occur (no direct access to the .dll)
    Build dependencies do not include all subprojects. So for instance when debugging, a clean was required first (partly hidden by the previous issue).

TranslationApp should be built last.

GitExtensions.exe should not have dependency to TranslationApp
TApp should have dependency to GE, VS Plugin, Setup (no need for all individual projects)
Setup had no dependency to VS plugins
@gerhardol gerhardol force-pushed the bugfix/n4269-project-dependencies branch from 86a6d1d to 644cfc9 Compare January 29, 2018 21:29
@gerhardol
Copy link
Member Author

Rebased and squashed, no changes was needed

After this, the maintenance that need to be done is to add new GEPlugins as dependencies for GitExtensions

@RussKie
Copy link
Member

RussKie commented Jan 29, 2018 via email

@gerhardol
Copy link
Member Author

I took a look at the second step I had worked on some time ago. Will submit a PR if this is merged.

https://github.com/gerhardol/gitextensions/tree/feature/n4269-build-dependency-pt2

After that, I would like to solve #4311 and #4312 and simplify the release process some. Seem to complicated now.

@RussKie
Copy link
Member

RussKie commented Jan 30, 2018 via email

@gerhardol gerhardol merged commit b490093 into gitextensions:master Jan 30, 2018
@gerhardol gerhardol mentioned this pull request May 11, 2018
Closed
@gerhardol gerhardol deleted the bugfix/n4269-project-dependencies branch June 5, 2018 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
👓 status: needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gerhardol @RussKie