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

CMake: remove obsolete references to files generated by scons #3550

Merged
merged 1 commit into from
Nov 20, 2021

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Jan 11, 2021

No description provided.

@github-actions github-actions bot added the build label Jan 11, 2021
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The files are still there on a developers machine.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 11, 2021

If someone still has old files on their machine that they have not cleaned up, that is their problem. That's not relevant to cpack which is executed on GH Actions always in a new VM.

@daschuer
Copy link
Member

This file is not only used by GitHub actions.
These extra lines do not hurt.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 11, 2021

What else uses these lines besides GitHub Actions?

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Developers are supposed to start the build from a clean checkout. We are not able to exclude any unknown files that might still lay around in the local file system. The common exclude patterns in CPACK_SOURCE_IGNORE_FILES are an acceptable exception.

The only alternative would be to explicitly define all files/patterns that should be included in the set. That would be my preferred approach to avoid unintended inclusions.

@daschuer
Copy link
Member

That is slightly different here. If you switch from to 2.2 and back the files are there.
These extra lines are helpful and do not hurt.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 11, 2021

By that logic we should still have these files in .gitignore but we have already removed those.

@daschuer
Copy link
Member

Ups ... I see that was done here: #2777
We may consider to revert this turns out to be an issue.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 11, 2021

Considering nobody has complained, I think there is no issue. It is unlikely anyone would accidentally commit scons files to 2.3 because they would be untracked files that would have to be intentionally staged. For CPack it is very unlikely to be a problem because CPack only needs to be run by GH Actions. There is no need for developers to run CPack locally unless they are working on packaging.

@Holzhaus
Copy link
Member

Holzhaus commented Jan 11, 2021

That is slightly different here. If you switch from to 2.2 and back the files are there.
These extra lines are helpful and do not hurt.

I thought we already decided that we don't want another 2.2 release. Is this really an issue?

@daschuer
Copy link
Member

It is probably no real issue but it is extremely annoying if the huge cache directory is packed, running your system out of space.
Compared to this keeping the ignore lines are also no issue.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 11, 2021

You are more likely to notice the build artifacts from 2.2 if they are not in .gitignore so they show up as untracked files in git status.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 15, 2021

ping

@daschuer
Copy link
Member

daschuer commented Jan 15, 2021

Let's close this. These lines are still helpful. The issue is, if one still has these file and likes to keep them, he as to re-add them and exclude his changes from commits. This is unlike a .git-ignore, where a user version exists that can be used. This is annoying. This comes with the cost of just three extra lines visual clutter.
There is no extra maintenance, because we will always need to maintain an exclude region in the CMakeList.txt

  • cache -> a biiiig directory, duplicate it will likely eat up all HD
  • *_build -> or old recommend cmake build folder, some users might not yet have moved to build, because there is no pressing reason
  • .sconf_temp -> we never want to see this folder in a package

@uklotzde
Copy link
Contributor

Let's separate those lines from the rest and leave a comment to remove them after some grace period, e.g. branching 2.4.0 beta

@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Apr 26, 2021
@Be-ing Be-ing changed the base branch from 2.3 to main November 20, 2021 11:50
@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 20, 2021

We now have a stable release without SCons. I don't think there's any reason to keep this legacy code.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, now that the 2.2 branch is no longer relevant the risk of problems after merging this is marginal. LGTM

@daschuer daschuer merged commit c175027 into mixxxdj:main Nov 20, 2021
@Be-ing Be-ing deleted the remove_scons_cpack branch November 20, 2021 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build stale Stale issues that haven't been updated for a long time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants