jamulus icon indicating copy to clipboard operation
jamulus copied to clipboard

Place build generated files in separate folders

Open pgScorpio opened this issue 2 years ago • 18 comments

Let's keep the root folder clean after a build...

Short description of changes Place generated moc and ui files in ./src/Generated and object files in ./obj so the jamulus root folder stays clean.

CHANGELOG:

Context: Fixes an issue? Not really

Does this change need documentation? What needs to be documented and how?

Status of this Pull Request

What is missing until this pull request can be merged?

Checklist

  • [ x] I've verified that this Pull Request follows the general code principles
  • [x] I tested my code and it does what I want
  • [x] My code follows the style guide
  • [x] I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • [x] I've filled all the content above

pgScorpio avatar Apr 08 '22 12:04 pgScorpio

We also might want to add these folders to .gitignore ?

pgScorpio avatar Apr 08 '22 12:04 pgScorpio

Thanks. Yes it was really annoying to have these files floating around in the root folder.

Maybe we could rename Generated to compiled/ or similar? Generated is a bit arbitary, imho? Also I just noticed that we use lower case folder names.

ann0see avatar Apr 08 '22 17:04 ann0see

Android failed. Maybe autobuild is no longer removing the files if we do an architecture change?

ann0see avatar Apr 08 '22 19:04 ann0see

Have all places been checked for potentially required updates regarding file inclusion/exclusion lists

That's exactly what we were struggeling with the last hour... Probably that's the issue why android fails.

ann0see avatar Apr 08 '22 20:04 ann0see

OK I've done some investigation, and I discovered a lot... I searched on several forums, encountering a lot of the same or similar problems, but no solutions that work on all build scenario's. All due to to some very strange behavior of qmake.

So I tried just not to apply these changes for android, and then, of course, android builds again, but I found out that this still does not give the expected results. Even without these changes most builds don't do what you would expect (like translation .qm files that are NOT placed in ./src/res/translation!, that only works if the build folder is ./ and it is not a multi target build).

But a lot of settings in the pro file won't work as expected, like:

  • DESTDIR and OBJECTS_DIR overrides any setting in the make files, but as so also overrides any debug/release or target dependent folders that would be used if it is not specified in the .pro file.
  • LRELEASE_DIR is used as a base folder (relative to the build folder!), but debug/release or target subfolders will be added automatically if LRELEASE_DIR is specified in the .pro file, though if not specified in the .pro file they will be placed in a corresponding build subfolder.
  • UI_DIR also is used as a base folder, but in the debug/release subfolders and will have (unnecessary ) subfolders for multi target builds.
  • MOC_DIR is also used as a base folder in the build folder, but won't have the (sometimes necessary) target subfolders, though if not specified in the .pro file the moc files will be placed in the correct build subfolders.

And all of these variables won't contain the used default value when examined in the .pro file (all empty).

Also building from Qt creator does never work as expected already (also even without these changes), since some folder settings in the .pro file are still ignored and/or do not behave as expected, especially when using "Shadow build" (and shadow build is default on in Qt Creator and this can't be controlled from any project configuration file).

So there is still a lot of investigation to be done, even when we do not apply this PR.

And so I have some questions to all of you:

  • Why are the translation_*.qm in the repo ? It seams they are re-created on every build (though not always at this location!)
  • Do you ever use Qt Creator to build? If yes do you use the default configuration ? If not what do you change ?
  • Do you ever build multiple, different, targets on the same machine ? What problems do you encounter with that ?
  • Who has experience in building android, how do you do it ? (Since my android builds work, but the ui is a mess, and I suspect the wrong UI files are used.)
  • Also for the other platform builds I'm curious who does build how (and why?, problems with other methods?)

pgScorpio avatar Apr 09 '22 22:04 pgScorpio

QM files are there due to compatibility reasons. There's work on not needing them https://github.com/jamulussoftware/jamulus/pull/2393

ann0see avatar Apr 10 '22 05:04 ann0see

Do you ever use Qt Creator to build? If yes do you use the default configuration ? If not what do you change ?

Probably @pljones is using Qt creator.

Do you ever build multiple, different, targets on the same machine ? What problems do you encounter with that ?

Not really, only 32 bit and 64 bit Windows. See the deploy_windows.ps1 script which basically just does a build via CLI

Who has experience in building android, how do you do it ? (Since my android builds work, but the ui is a mess, and I suspect the wrong UI files are used.)

@sthenos @j-santander probably have experience with building for Android

Also for the other platform builds I'm curious who does build how (and why?, problems with other methods?)

I mostly just use the command line to build. I'm mainly building on Linux which works without much setup. macOS is also quite ok (mostly everything works via Xcode). Windows was a pain for me (and I just ran the deploy_windows.ps1 script as a result) Android was done in a clean docker container with Ubuntu, iOS also via Xcode

Edit: I'd suggest not to discuss that in this PR but in a issue of the type I mentioned: open a PROJECT: Sound-Redesign issue with a checklist, what you plan to change etc. Then you can easily ask for help there and information isn't scattered around multiple PRs

ann0see avatar Apr 10 '22 05:04 ann0see

I'd suggest not to discuss that in this PR but in a issue of the type I mentioned: open a PROJECT: Sound-Redesign issue with a checklist, what you plan to change etc.

But these problems are not sound related at all. I will create a new sound-redesign issue, but I like to have a solid foundation to work on, so I would like to to have these build issues solved, or at least clear to me, first. Also the sound-redesign depends on several other issues to be solved/implemented first (like moving the sound related files, implementing global message boxes and commandline parameters).

QM files are there due to compatibility reasons. There's work on not needing them https://github.com/jamulussoftware/jamulus/pull/2393

Thanks for that link, several problems with translation mentioned in that issue seem to be related to the unexpected qmake build behavior I mention here!

pgScorpio avatar Apr 10 '22 14:04 pgScorpio

How should we proceed here? Can you live with the current behaviour? Strictly speaking the build works – even if not perfect, so moving the generated files is nice but not blocking.

ann0see avatar Apr 14 '22 06:04 ann0see

How should we proceed here? Can you live with the current behaviour?

Yes I can, since there is a work-around: Do not build from the root folder but from a build folder! (Basically the principle deploy_windows also uses.)

cd build
mkdir <your build folder>
cd <your build folder>
qmake ../../Jamulus.pro
make
make install
make clean # or make distclean

Strictly speaking the build works – even if not perfect, so moving the generated files is nice but not blocking.

Yes, strictly speaking all builds work, but maybe we should have a closer look, since not all works as you would expect. Especially translation files (also related to #2393), some (minor) issues with the android build (i.e. copying sound source files to the package folder) and make clean that does not always clean everything (i.e. generated ui_*.h and *.qm files). Further DISTFILES is set for all builds, while it's only valid for a unix target. And messages are shown 3 times for debug_and_release builds (default on windows), etc...

I already had a thorough look into this and I already have solutions for most issues. The question is do we want to change it or not ? (for those who are interested, have a look at my qmake-issues-testing branch, which has almost everything implemented)

pgScorpio avatar Apr 14 '22 17:04 pgScorpio

Did you do a git rebase? I think there's something wrong with this PR (since the commits from master show up).

ann0see avatar Apr 14 '22 20:04 ann0see

Did you do a git rebase? I think there's something wrong with this PR (since the commits from master show up).

Yes I just did a rebase, but not on this branch, so no idea what is happening here.

On my own repo I see "gilgongo authored and pgScorpio committed 3 hours ago" ??? But also "This branch is 5 commits ahead, 3 commits behind jamulussoftware/jamulus:master." So it does not seem to be be just rebased...

pgScorpio avatar Apr 14 '22 21:04 pgScorpio

Maybe you we're still on the wrong branch? Could you please show the commands you used?

ann0see avatar Apr 15 '22 05:04 ann0see

Maybe you we're still on the wrong branch? Could you please show the commands you used?

I'm not quite sure. I was working on my 'global-messages' branch, and wanted to do some testing on my linux vm... so on my vm I did: (As far as I can remember)

git checkout global-messages  # but it complained the branch was not found, so I did
git fetch remote
git checkout global-messages  # this one succeeded but it complained, I don't remember exactly what about, but I think I did:
git fetch upstream
git rebase -i upstream/master # which did some updates, after this git status was OK and I did my testing and a bugfix here.., then I did:
git add src/messages.h
git commit
git push

pgScorpio avatar Apr 15 '22 12:04 pgScorpio

Git: It looks like two commits have been cherry-picked from master and/or those commits (instead of this PR) have been rebased on top of master. Checking out this PR's branch and running git rebase upstream/master fixes this though (tried locally, but didn't push). The above git commands look like they should not have had an effect on this PR's branch (generated-files).

PR: Glad you've got it to work! I'm starting to become a bit skeptical of the implementation though. The PR adds rather lots of logic and introduces usage of Qt-internal variables (as the comments say). I'm wondering if we're still on the right track. Are there any other popular projects doing the same? I'm a bit wary of ending up with a rather exotic Jamulus.pro setup which may show different issues later (thinking of Qt6 compatibility, as just one example).

hoffie avatar Apr 16 '22 15:04 hoffie

Git: It looks like two commits have been cherry-picked from master and/or those commits (instead of this PR) have been rebased on top of master.

Yes I did cherry pick, but that was on another branch. Amazingly these are the two I didn't pick then ????

Checking out this PR's branch and running git rebase upstream/master fixes this though (tried locally, but didn't push). The above git commands look like they should not have had an effect on this PR's branch (generated-files).

Thanks for the advise. The rebase indeed skipped the two cherrypicks, but unfortunately the push failed: Peter@PC10 MINGW64 /f/Repos/pgScorpio/jamulus (generated-files)

$ git rebase upstream/master
warning: skipped previously applied commit 8191032b
warning: skipped previously applied commit d4619d5b
hint: use --reapply-cherry-picks to include skipped commits
hint: Disable this message with "git config advice.skippedCherryPicks false"
Successfully rebased and updated refs/heads/generated-files.
Peter@PC10 MINGW64 /f/Repos/pgScorpio/jamulus (generated-files)
$ git push
To github.com:pgScorpio/jamulus.git
 ! [rejected]          generated-files -> generated-files (non-fast-forward)
error: failed to push some refs to 'github.com:pgScorpio/jamulus.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
Peter@PC10 MINGW64 /f/Repos/pgScorpio/jamulus (generated-files)

So I did a pull but now git status shows:

$ git status
On branch generated-files
Your branch is ahead of 'origin/generated-files' by 9 commits.
  (use "git push" to publish your local commits)

nothing to commit, working tree clean

So with "9 commits ahead" I'm reluctant to push, because it's not what I would expect and I don't want to mess up things even more. P.S. I think this is exactly the problem caused by rebasing a local branch against upstream while not rebasing origin/master against upstream first. (As also discussed here )

pgScorpio avatar Apr 16 '22 18:04 pgScorpio

PR: Glad you've got it to work! I'm starting to become a bit skeptical of the implementation though. The PR adds rather lots of logic and introduces usage of Qt-internal variables (as the comments say).

The comment says "undocumented", in quotes, since they are not in the Qt documentation here but most of them are documented here and others I found on the Qt forum

I'm wondering if we're still on the right track. Are there any other popular projects doing the same?

Yes. I found the same android solution on multiple different Qt forums....

I'm a bit wary of ending up with a rather exotic Jamulus.pro setup

So do I, since the Qt documentation is not very clear.. For some variables they state clearly when introduced, but for most it's just a guess. Also many variables do not seem to do as expected from the documentation (Especially for multi target builds). And also many variables have no 'current' variable set, but, if set in the pro file, they will override the 'current value' So yes, it took a lot of experimenting and logging to find out how things really work/behave...

which may show different issues later (thinking of Qt6 compatibility, as just one example).

I'm afraid there are already some issue's with the current implementation... and some of them are still unclear to me. And yes, Qt 6 will introduce more compatibility issues...

pgScorpio avatar Apr 16 '22 18:04 pgScorpio

Bumping as notabug.

pljones avatar Aug 29 '22 16:08 pljones

On the Qt Creator front: it doesn't necessarily dump files in the root of the project tree: you can tell it where to put them (and they go in my temp directory). That means you just get a Jamulus.pro.user* file, I think.

pljones avatar Oct 01 '22 10:10 pljones

That sounds great. Can we add something like that to Jamulus.pro then?

ann0see avatar Oct 01 '22 10:10 ann0see

That sounds great. Can we add something like that to Jamulus.pro then?

Hm, I don't think so - it looks Qt Creator-specific:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE QtCreatorProject>
<!-- Written by QtCreator 4.11.0, 2022-10-01T12:16:01. -->
<qtcreator>
...
 <data>
  <variable>ProjectExplorer.Project.Target.0</variable>
  <valuemap type="QVariantMap">
...
   <valuemap type="QVariantMap" key="ProjectExplorer.Target.BuildConfiguration.0">
    <value type="QString" key="ProjectExplorer.BuildConfiguration.BuildDirectory">G:/build/Jamulus-Desktop_Qt_5_12_2_MSVC2017_64bit_static-Release</value>
...
   </valuemap>
...
  </valuemap
...
 </data>
...
</qtcreator>

Unless there's a flag that can be passed to qmake to tell it the BuildDirectory.

	D:\Qt\5.12.2\msvc2017_64-static\bin\qmake.exe -o Makefile O:\git\Jamulus-wip\Jamulus.pro -spec win32-msvc
...
	cl -c -nologo -Zc:wchar_t -FS .... -EHsc -D_UNICODE .... -DNDEBUG -IO:\git\Jamulus-wip .... -Irelease -I. -ID:\Qt\5.12.2\msvc2017_64-static\mkspecs\win32-msvc -Forelease\ @D:\TEMP\multicolorled.obj.4504.812.jom
...

So I think it's in how the compiler is configured in the generated make file or *.jom file....

pljones avatar Oct 01 '22 11:10 pljones

On the Qt Creator front: it doesn't necessarily dump files in the root of the project tree: you can tell it where to put them

This is only true if "Shadow build" is turned on in QtCreator (the default) and still than files are not all located in logical subfolders in the created build folder.

But for (almost?) all generated files a destination subfolder can be set in the .pro file though.

pgScorpio avatar Oct 01 '22 17:10 pgScorpio

This looks to be going stale. I'll move to Triage and drop the milestone. We can review priorities again at some point but with waning interest in Jamulus, it's less likely "nice to haves" will get done above outright bugs (and things I'm interested in, as I'll likely Carry on Coding 😄).

pljones avatar Apr 19 '23 15:04 pljones

I think we should open an issue on this PR and then close it as I don't see that we can really proceed here.

ann0see avatar Jun 26 '23 21:06 ann0see

Issue #3092 opened. Thus this PR is closed as stale

ann0see avatar Jul 01 '23 19:07 ann0see