-
Notifications
You must be signed in to change notification settings - Fork 238
Place build generated files in separate folders #2588
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
Conversation
Let's keep the root folder clean after a build... Place moc and ui files in ./src/Generated and object files in ./obj
|
We also might want to add these folders to .gitignore ? |
|
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
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is not that important, but I'd like something lower case at least
|
Android failed. Maybe autobuild is no longer removing the files if we do an architecture change? |
hoffie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of keeping the repo root clean.
- I'm totally unsure about the directory names though. Is there some kind of standard or best-practice?
- As @ann0see noted, I'd also go with a lowercase name to match what's there.
- Does it have to be in src/? Could we place all generated files (object files and others) in a single dir? Something like build/ would sound more natural to me (although we should consider that build scripts also use this directory...)?
- Have all places been checked for potentially required updates regarding file inclusion/exclusion lists (.github, tools, Jamulus.pro)?
That's exactly what we were struggeling with the last hour... Probably that's the issue why android fails. |
|
OK I've done some investigation, and I discovered a lot... 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:
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:
|
|
QM files are there due to compatibility reasons. There's work on not needing them #2393 |
Probably @pljones is using Qt creator.
Not really, only 32 bit and 64 bit Windows. See the deploy_windows.ps1 script which basically just does a build via CLI
@sthenos @j-santander probably have experience with building for Android
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 |
But these problems are not sound related at all.
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! |
|
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. |
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.)
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 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 ? |
|
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" ??? |
|
Maybe you we're still on the wrong branch? Could you please show the commands you used? |
I'm not quite sure. |
This solution should work for android builds too
(I knew it. But trying to take too small steps I forgot about it...)
|
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 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). |
Yes I did cherry pick, but that was on another branch. Amazingly these are the two I didn't pick then ????
Thanks for the advise. The rebase indeed skipped the two cherrypicks, but unfortunately the push failed: So I did a pull but now 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. |
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
Yes. I found the same android solution on multiple different Qt forums....
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'
I'm afraid there are already some issue's with the current implementation... and some of them are still unclear to me. |
|
Bumping as notabug. |
|
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. |
|
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 So I think it's in how the compiler is configured in the generated make file or |
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. |
|
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 😄). |
|
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. |
|
Issue #3092 opened. Thus this PR is closed as stale |
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