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

Cleanup GitHub Root Dir #2201

Closed
tresf opened this issue Jul 21, 2015 · 21 comments
Closed

Cleanup GitHub Root Dir #2201

tresf opened this issue Jul 21, 2015 · 21 comments

Comments

@tresf
Copy link
Member

tresf commented Jul 21, 2015

We have a lot of files in the root of our source tree and many of them are no longer used, redundant, or are input files for cmake.

This is a thread to talk about cleaning these up. Feedback needed.

The first column is Keep (green), Move (check) or Delete (blank)

Keep? File Done? Notes
.gitattributes Keep
.gitignore Keep
.gitmodules 👍 Was used for Zyn submodule. Empty now. Safe to remove.
.mailmap 👍 Move, if cmake/our code allows it. Keep per #2210 (comment)
.travis.yml Keep
☑️ AUTHORS 👍 Better suited for data docs
CMakeLists.txt Keep
☑️ COPYING 👍 Rename to LICENSE.txt?
INSTALL 👍 Kept and merged w/ Qt5 Can we just link to the wiki?
INSTALL.Qt5 👍 Can we just link to the wiki? Merged w/ INSTALL
README 👍 Can we just keep the markdown file?
README.md Keep, needed for GitHub.
TODO 👍 Safe to remove, per #1964
☑️ build_mingw32 build_mingw64 👍 Move to data or cmake/scripts. Update travis, Windows wiki.
configure 👍 Not used anymore. Safe to remove.
☑️ lmms.1 👍 Move to data or cmake? docs. Assumes source allows.
☑️ lmms.rc.in 👍 Move to data or cmake? Assumes source allows.
☑️ lmms.spec.in 👍 RedHat/Fedora .spec RPM file? Move to data or cmake? Assumes source allows.
☑️ lmmsconfig.h.in 👍 Move to data or cmake? src. Assumes source allows.
☑️ lmmsversion.h.in 👍 Move to data or cmake? src. Assumes source allows.
@softrabbit
Copy link
Member

A few opinions.

Keep? File Done? Notes
COPYING I'm pretty sure COPYING is a GNU recommendation for naming the file containing the GPL. Not that it binds LMMS in any way, but it seems to be pretty standard.
INSTALL, INSTALL.Qt5 At the very least, these could be fusioned into one INSTALL.
README Out of date? Qt 4.6 is required now, file says 4.3. I expect other details to be a bit behind as well. And two files is a bit excessive.
README.md Maybe add useful info from README; e.g. library dependencies are only listed there.
☑️ build_mingw32, build_mingw64 These mingw scripts could probably be incorporated in CMakeLists.txt.
configure Has pointed towards INSTALLsince 2008. Kill it.
☑️ lmms.1 Move to data or doc, man pages are documentation.

@tresf
Copy link
Member Author

tresf commented Jul 22, 2015

Some replies...

Keep? File Done? Notes
COPYING [...] COPYING is a GNU recommendation Yes it appears to be. Here's a decent article on it. Seems to be personal preference. http://stackoverflow.com/a/5678716/3196753
README.md Maybe add useful info from README; e.g. library dependencies are only listed there. Hmm... Do people still use instructions from a text files, or can we safely assume they go to our well-written wiki first? I agree there is a nice dependency listing, but what good is that in the README? Shouldn't that be in the installation steps or is my thinking wrong?
☑️ build_mingw32, build_mingw64 These mingw scripts could probably be incorporated in CMakeLists.txt. Not sure if this is possible per http://www.cmake.org/pipermail/cmake/2008-April/021122.html

@Wallacoloo
Copy link
Member

I feel very lowly in neglecting the fancy formatting you've applied, but here's my weigh-in:

build_mingw32/64 could possibly be moved into data/scripts - at least, that's where we have create_apple_bundle.sh.in etc and those seem related.

I think we can safely delete README, as well as both of the INSTALL files. I'm fond of a "don't repeat yourself" approach - all the information is already present in the wiki, where it is also organized better than a simple text file and more easily kept up to date.

I prefer LICENSE over COPYING, as a license is exactly what the GPL is, and the GPL covers more than just your permissions to copy (e.g. permissions to embed, modify, etc). While you're at it, I would gladly turn a blind eye if you accidentally replaced it with a less horrid, more permissive license.

Beyond moving lmms.spec.in elsewhere, I've proposed a PR to remove its "Changelog" section as well: #2203.

I don't think lmms.1 should be moved to cmake - that directory looks to be only cmake code. I think it would be more appropriate to move this and lmms.spec.in somewhere into data.

A natural place for lmmsconfig.h.in and lmmsversion.h.in seems like the root of the src directory, as these are used to generate header files which are then used exclusively by other files in the src directory.

I can't say much about lmms.rc.in, because I can't actually tell what it does. Is this info for the windows installer?

@curlymorphic
Copy link
Contributor

Most of my thoughts have already been covered, so no need to repeat them.

Regarding the ReadMe file. I feel this should contain current building instructions, as it is possible for the source code to come from third party origins with no links to our wiki

@tresf
Copy link
Member Author

tresf commented Jul 23, 2015

I can't say much about lmms.rc.in, because I can't actually tell what it does. Is this info for the windows installer?

NSIS handles the installer, it appears this information is for embedding necessary information into the EXE and DLLs. This is mostly used in Windows to fill the "Properties" screen so that when you right click a file, you can get detailed version information.

As with all of our .in files, CMake processes it first to fill in the necessary fields, then it's passed to the windres utility.

Here's where the input file is mentioned:
https://github.com/LMMS/lmms/blob/master/CMakeLists.txt#L371

And our tarball script references it as well here (this tarball script looks like it could be cleaned up as well):
https://github.com/LMMS/lmms/blob/master/CMakeLists.txt#L501

@tresf
Copy link
Member Author

tresf commented Jul 23, 2015

I prefer LICENSE over COPYING, as a license is exactly what the GPL is, and the GPL covers more than just your permissions to copy

Agreed.

A natural place for lmmsconfig.h.in and lmmsversion.h.in seems like the root of the src

Since the root of src is empty, I tend to agree. BTW, is there a reason why these files can't be merged into one?

build_mingw32/64 could possibly be moved into data/scripts - at least, that's where we have create_apple_bundle.sh.in etc and those seem related.

Yeah, our build toolchains need a home I think. I'm still not convinced that cmake isn't the best place, as they're all tied to cmake in one way or another and they aren't data in the terms of a resource file that the software needs to run (such as a pixmap).

Since the root of cmake is empty, I say we create a new scripts folder in there for all building and packaging needs and consider removing it from data/scripts. To me, data/scripts would be more relevant for something like a lmms launcher script that is actually deployed to the machine (Such as how Firefox uses a launcher script on Fedora in /usr/bin), but I'd like to hear more opinions. 👍

tresf added a commit to tresf/lmms that referenced this issue Jul 25, 2015
@tresf
Copy link
Member Author

tresf commented Jul 25, 2015

Please see #2210 (vs. 2201? how confusing! 😈 )

What I've done is remove what we've discussed, kept what we've discussed and only changed the source location of the files. This means cmake will copy them to the same place it always has for building, but it will look for them in their new homes. On a side note, we should eventually make the build process directory consistent with the source structure, but I'd rather do that in a separate cleanup effort if everyone doesn't mind as #include lmmsversion.h versus #include foo/lmmsversion.h could be much larger impacting to our source tree and out of scope for this effort I feel.

I've decided on cmake/scripts as the home for our build scripts. LMK if you disagree. I will eventually be moving the Apple build scripts there as I feel they're better suited in the cmake area of our structure as they're directly related to building and packaging (even if their reliance on cmake is minimal, it seems better than the data directory).

I'm also optimistic that travis will help iron out the kinks. As always, feedback appreciated. 👍

@tresf
Copy link
Member Author

tresf commented Jul 25, 2015

Ok, I had missed a few of the .hidden ones, but now the PR and table have been updated to cover them all now. Please let me know if there are any objections. I'll squash the commits once I get the green 🚦

New root directory:

.travis 
buildtools
cmake
data
doc
include
plugins
src
tests
.gitattributes
.gitignore
.travis.yml
CMakeLists.txt
INSTALL
LICENSE.txt
README.md

@softrabbit
Copy link
Member

I've decided on cmake/scripts as the home for our build scripts. LMK if you disagree.

Why not buildtools? It sits there in the root directory, clearly visible with a descriptive name. OK, anything that is run from cmake fits in cmake/scripts as I see it, but at least the mingw_* scripts are pretty much the opposite, they invoke cmake.

@tresf
Copy link
Member Author

tresf commented Jul 29, 2015

at least the mingw_* scripts are pretty much the opposite, they invoke cmake.

I like the buildtools home, thanks. buildtools/scripts or simply buildtools?

@tresf
Copy link
Member Author

tresf commented Jul 29, 2015

Quoting a comment from the PR:

This breaks any git logs for those who have not set their git config to point to the new .mailmap location in doc. Let's keep the .mailmap file in the root directory.

Duly noted @lukas-w, thanks.

-Tres

@Umcaruje
Copy link
Member

I'd vote for simply buildtools.

tresf added a commit to tresf/lmms that referenced this issue Jul 30, 2015
tresf added a commit to tresf/lmms that referenced this issue Jul 30, 2015
@tresf
Copy link
Member Author

tresf commented Jul 30, 2015

I think all concerns have been addressed via tresf@95f0b29. Let me know if I missed something. If not, I'll squash down the commits and merge.

@tresf
Copy link
Member Author

tresf commented Jul 30, 2015

@softrabbit

Why not buildtools? It sits there in the root directory, clearly visible with a descriptive name. OK, anything that is run from cmake fits in cmake/scripts as I see it, but at least the mingw_* scripts are pretty much the opposite, they invoke cmake.

To this point, should nsis/FileAssociation.nsh be removed from the cmake directory?

-Tres

@tresf
Copy link
Member Author

tresf commented Aug 2, 2015

@lukas-w any opinions on a permanent place for the build_mingw32, build_mingw64 scripts? We have some other scripts -- like the ones we use for apple bundling and packaging and our new and upcoming msys scripts -- that I'd also consider moving to a similar home.

@lukas-w
Copy link
Member

lukas-w commented Aug 6, 2015

buildtools sounds fine to me. I'd like buildscripts or just scripts as well, maybe a bit more. Note that the (currently) only file in buildtools, bin2res.cpp, will be removed when merging #1891 so there's no real point in using that directory just because it's already there.

@tresf
Copy link
Member Author

tresf commented Aug 6, 2015

there's no real point in using that directory just because it's already there.

Right... to me it all seems related to how we build, so cmake seems about as relevant as any, especially considering we have the NSIS script in there.

-Tres

@Wallacoloo
Copy link
Member

Wallacoloo commented Aug 6, 2015 via email

@tresf
Copy link
Member Author

tresf commented Aug 7, 2015

Warning, TL;DR...

I still like the cmake folder because it is relevant to what the scripts are for, especially considering we currently have our NSIS packaging script in there.

This is what I propose:

  • We switch all unix build scripts to use bash by default using #!/bin/bash rather than #!/bin/sh and append the.sh file suffix for consistency.
cmake/build_*.sh        # all non-standard build scripts
cmake/install_*.sh      # all non-standard install scripts (Apple needs one of these for bundling)
cmake/package_*.sh      # all non-standard package scripts (Apple needs one of these for dmg'ing)

#    NOTE: 
#
#    Ideally, we'd do everything automatically through CMake
#         e.g. make 
#              make install
#              make package
#              ..etc
#    BUT... some items such as mingw toolchain and Apple packaging currently
#    have good reasoning on remaining external scripts -- at least for now.

For example:

cmake/build_mingw.sh    # builds for win64 on msys2 and mingw.  win32 can be forced via flag
cmake/install_mac.sh    # install (bundle) target for the current osx platform
cmake/package_mac.sh    # package (dmg) for the current osx platform

But this begs the question, where do the configure files (.in) and package specific icon/installer background files reside? I vote they go into a platform-specific folder via:

cmake/linux/lmms.desktop        # desktop shortcut for linux installations
cmake/linux/lmms.xml            # file association for linux installations
cmake/linux/lmms.png            # desktop icon for linux installations
cmake/linux/application-x-lmms-project.svg  # project icon for linux installations
cmake/linux/lmms                # seriously, WTF is this `data/lmms` file even for?

cmake/nsis/FileAssociation.nsh  # helper script for nsis (win32, win64) packaging
cmake/nsis/nsis_branding.bmp    # artwork for nsis installer
cmake/nsis/lmms.ico             # desktop icon for nsis installer

cmake/mac/install_mac.sh.in     # install template for the current osx platform
cmake/mac/package_mac.sh.in     # package template for the current osx platform
cmake/mac/lmms.plist.in         # installation instructions template for osx
cmake/mac/project.icns          # project icon for osx
cmake/mac/lmms.icns             # dock icon for osx installations

cmake/msys/msys_helper.sh       # helper script for msys2 environment
cmake/msys/fetch_ppa.sh         # helper script for msys2 environment
cmake/msys/extract_debs.sh      # helper script for msys2 environment

Edit: Note, if you look at the data directory we have today, it will be much more organized after this as it is currently filled with build-specific files, which I find a bit misleading considering we use the data directory for static code-seeking resources (themes, presets, blah) as well and this would help define what data is actually there for, rather than a miscellaneous catch-all folder.

@Wallacoloo
Copy link
Member

Wallacoloo commented Aug 7, 2015 via email

tresf added a commit to tresf/lmms that referenced this issue Aug 7, 2015
@tresf
Copy link
Member Author

tresf commented Aug 12, 2015

I've cleaned up the data directory and also taken some time to go through the CONFIGURE (.in) files and force them to use variables instead of hard-coded values... I haven't done source files, just cmake and resource files for now. #2210/files

For example...

  • Before: LMMS
  • After: ${PROJECT_NAME_UCASE}

I've also switched to spaces for formatting on some vertically aligned areas to fix improper displaying on certain editors.

I'd like to get this merged soon, so any and all feedback is appreciated.

tresf added a commit to tresf/lmms that referenced this issue Aug 12, 2015
tresf added a commit to tresf/lmms that referenced this issue Aug 12, 2015
Per LMMS#2201

Formatting fix
tresf added a commit to tresf/lmms that referenced this issue Aug 13, 2015
Cleans up the root directory of the GitHub source tree and starts to separate platform-specific installing and packaging logic from the master CMakeLists.txt.  Closes LMMS#2201
tresf added a commit to tresf/lmms that referenced this issue Aug 13, 2015
Cleans up the root directory of the GitHub source tree and starts to separate platform-specific installing and packaging logic from the master CMakeLists.txt.  Closes LMMS#2201
ThomasJClark pushed a commit to ThomasJClark/lmms that referenced this issue Sep 12, 2015
Cleans up the root directory of the GitHub source tree and starts to separate platform-specific installing and packaging logic from the master CMakeLists.txt.  Closes LMMS#2201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants