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

Fix css being versioned, rename to custom.css.sample #1540

Closed
wants to merge 2 commits into from

Conversation

amcolash
Copy link
Contributor

At the current moment, the file custom.css is versioned in git but also has an entry in the .gitignore file. Since it was versioned first, modifications to said file make it look like there are changes from git. This PR renames that file to custom.css.sample to match with the config file. If a user wants custom css, they can make a copy of the file and no more changes from the git perspective!

@MichMich
Copy link
Collaborator

Hi, not sure what the issue is, since the current situation has never changed and I never got any git issues (neither did I get any reports about this in the past.) Could you elaborate on the issue?

@amcolash
Copy link
Contributor Author

If I modify custom.css, git says it is under version control and I have uncommitted changes. I would like to take this file out of version control and instead have the sample file similar to config. I hope that helps explain better.

@MichMich
Copy link
Collaborator

That's weird. I've changed the custom.css as well and don't get the git warning. We might want to check on the forum if more users are experiencing this issue.

@roramirez
Copy link
Contributor

I've check about this thing time ago

https://forum.magicmirror.builders/topic/1508/custom-css

The really problem can be is for new updates the users with changes in own custom.css can be a completly issue here with this change.

@amcolash
Copy link
Contributor Author

amcolash commented Feb 2, 2019

Yup that was the issue I was noticing. Also, if I make a PR to the MM repo, I can't just do git add ., instead need to exclude the css from commits

@MichMich
Copy link
Collaborator

So, I'm not sure what to do with this pr. It seems I'm getting mixed feedback on what happens whenever custom.css was changed. I modified the file on my own installation and never have any git issues. Can anyone please explain in detail what happens and how this produces any problems? It seems I'm not getting it ... 🙃

@amcolash
Copy link
Contributor Author

I wonder if you have excluded that file from being further tracked (with something similar to https://stackoverflow.com/a/39776107/2303432).

Try running git ls-files -v . | grep ^S, or cloning the repo to a new location, editing and then checking git status.

In my own MM, if I modify the file and then run git status I see that the file has been modified. This causes issues since it is tracked (pulls and commits can be painful)

@fewieden
Copy link
Contributor

@amcolash maybe your system is having problems interpreting the gitignore file.

# Ignore changes to the custom css files.
/css/custom.css

Can you try to remove the leading / before css and commit the gitignore. Check afterwards if git status is still reporting a change in the custom css file.

@MichMich
Copy link
Collaborator

Hi @amcolash, it seems you are right. Changes are noted in git status.

Your solution seems ok, but I have two issues:

  • What will happen to the custom.css file for users that already use it and update to this change?
  • We might want to add a step to the installer or npm install that automatically creates the custom.css copy to make things easier for beginners.

@amcolash
Copy link
Contributor Author

amcolash commented Mar 1, 2019

  1. I just tested checking out to a point before my changes, modified the file, then checked out my changes and got a dreaded error: Your local changes to the following files would be overwritten by checkout. I am not too sure if there is any way around this - maybe some hackish way in an install script as part of an upgrade step, but I don't prefer that option.

  2. That should be fairly straightforward, just copy the sample file to the base file i.e. cp custom.css.sample custom.css. Whatever is done however, it should stay in sync with the js counter-part since these 2 configurations operate very similarly from a user's point of view.

@fewieden
Copy link
Contributor

fewieden commented Mar 2, 2019

It's probabl worth it investigating upfront why the gitignore rule is not working in the first place

@amcolash
Copy link
Contributor Author

amcolash commented Mar 6, 2019

It's not working since the css file was added before the ignore file I would assume - .gitignore only ignores new files.

Per git docs: A gitignore file specifies intentionally untracked files that Git should ignore. *Files already tracked by Git are not affected*

https://git-scm.com/docs/gitignore

@MichMich
Copy link
Collaborator

MichMich commented Mar 7, 2019

  • I just tested checking out to a point before my changes, modified the file, then checked out my changes and got a dreaded error: Your local changes to the following files would be overwritten by checkout. I am not too sure if there is any way around this - maybe some hackish way in an install script as part of an upgrade step, but I don't prefer that option.
    What if we remove any tracking of the custom.css file using git rm?

@MichMich
Copy link
Collaborator

Anyone know if there is a way to remove custom.css out of the git tracking without removing the file during updates of existing installations?

@ecampidoglio
Copy link

@MichMich I think what you're looking for is the skip-worktree bit:

git update-index --skip-worktree path/to/custom.css

From the documentation:

When reading an entry, if it is marked as skip-worktree, then Git pretends its working directory version is up to date and read the index version instead.

Here's the interesting part:

The working directory version may be present or absent. If present, its content may match against the index version or not. Writing is not affected by this bit, content safety is still first priority.

This means that those who have the custom.css file in their working directory (i.e. they cloned the repo before it was ignored) will keep it with whatever local modifications they made, but the file will no longer show up as modified when doing git status.

The downside is that the skip-worktree bit is applied to the index, which means it can't be pushed to a remote repository. In other words, users with a locally modified custom.css file will have to run the git update-index command on their own.

I hope this helps.

@MichMich
Copy link
Collaborator

Hi @ecampidoglio thanks for your suggestion.

I really hope we can find a solution which doesn't require any user to run additional commands. But I'm afraid this is probably impossible. If we do expect users to run commands, it might be better to completely remove custom.css from the git history renaming the file to the custom.css.sample (and add custom.css to gitignore.

In that case users need to backup their custom.css before upgrade (git pull). And can then put it back in place after upgrade. From then on, this issue should be solved. Right?

@sdetweil
Copy link
Collaborator

better to phase out the single file and migrate module css changes to their own file. the module loader could also load the 'modulename.css' in the module folder (just picking a predefined name). so no module code would have to be changed

@MichMich
Copy link
Collaborator

the custom.css is not specifically designed for changing modules. It's to allow users to change the look and feel of their mirror. IE: adding a background image, changing the paddings, changing font sizes, etc.

@ecampidoglio
Copy link

@MichMich

In that case users need to backup their custom.css before upgrade (git pull). And can then put it back in place after upgrade. From then on, this issue should be solved. Right?

Correct. One thing worth point out is that, since the custom.css file was ignored after it had been committed, in order to delete it from the repo you need to tell Git to remove its path from the index.

You can do that by using the --cached option of git rm:

git rm --cached path/to/custom.css
git commit -m "Stop tracking the custom.css file"

Note that the documentation will tell you this:

Working tree files, whether modified or not, will be left alone.

Which can be a little misleading — it is true that running this command won't remove the file from the working directory; however, it will be removed from the working directory of whoever pulls the commit. Hence, those who wish to keep their local modifications, should create a copy of it prior to pulling, as you said.

@MichMich
Copy link
Collaborator

MichMich commented Jun 18, 2019

@ecampidoglio, thanks! I think this is the direction we should go. But it might be a good idea to think of a solution that prevents accidental deletion of the file.

  • Is it possible to automatically run a command prior to a pull? If so, this command could make a backup.
  • Is it possible to do a check before pulling, something like: "It seems you have a modified custom.css in place. Please backup this file first."
  • We could implement an auto backup feature in the coming version, and remove the file in the next version. That way, chances are the file is backed up. (Except when a user skips a version).
  • We could add an npm run update feature to the coming release which takes care of this issue.

Any other idea, anyone? Preferably as simple as possible.

@sdetweil
Copy link
Collaborator

run update seems a good choice, as it could also look for modules that that need refreshing, and upgrade them (npm install, not git pull) ... seems this is a constant problem for users..

@MichMich
Copy link
Collaborator

The only issue is that if a user does git pull manually, the custom.css will still be deleted ...

@sdetweil
Copy link
Collaborator

you should change the name, so the git pull doersn't overlay, and add code to rename it

@MichMich
Copy link
Collaborator

Not sure if that will work, since we still need to remove custom.css from the git repo.

@mrvanes mrvanes mentioned this pull request Jul 18, 2019
@mrvanes
Copy link
Contributor

mrvanes commented Jul 21, 2019

You should rename custom.css to something like custom.css.sample (which causes git to untrack custom.css) and have run-start.sh create/copy it if it's absent, that's all.

@roramirez
Copy link
Contributor

@mrvanes This get a good point. I think we need a migration plan (pain). First add Warning if the file is tracking in the current local repository and give some instructions to keep out from there (locally) and keep this behavior almost one release more, also run-start-sh could do this work. After this merge this change into the current master.

@MichMich if this is good for you, I'll could to it.

@MichMich
Copy link
Collaborator

@roramirez Yes, feel free to send a pr which does the first step (in 2.9.0). In 2.10.0 or later we can do the next step.

@sdetweil
Copy link
Collaborator

I have an upgrade script just about ready for testing. It saves/restores custom.css

roramirez added a commit to roramirez/MagicMirror that referenced this pull request Jul 28, 2019
On the next release the css/custom.css will rename to
css/custom.css.sample

This change run git instructions to detach the file from own local
repository. This instructions are called in untrack-css.sh file from
run-start.sh and npm postinstall step

Reference MagicMirrorOrg#1540
@roramirez
Copy link
Contributor

@sdetweil please check the #1744 if you can improve with your script.

roramirez added a commit to roramirez/MagicMirror that referenced this pull request Jul 28, 2019
On the next release the css/custom.css will rename to
css/custom.css.sample

This change run git instructions to detach the file from own local
repository. This instructions are called in untrack-css.sh file from
run-start.sh and npm postinstall step

Reference MagicMirrorOrg#1540
@sdetweil
Copy link
Collaborator

@roramirez
Copy link
Contributor

I've take a fast view to your scripts and there some interesting ideas. Maybe you could clean and isolated from external dependencies and send a propose to include as "update-script.sh" into installers directory

@sdetweil
Copy link
Collaborator

Isolate? Clean?

I intend to submit once tested more
I also have updated installer, and a pm2 only script.

@roramirez
Copy link
Contributor

I mean the scripts you show there are external dependencies (in Dropbox). If you want add a script like update_script should be a separate from the actually installer because this now is working and a massive change will come a some troubles.

@sdetweil
Copy link
Collaborator

ok, of course.. once tested some more I will submit a PR to include them in the release..

the current installer has a bunch of problems, which is why i have updated it.

install is create new installation
update is to get to next version
pm2 fix is to add/update/fix pm2 setup

@stale
Copy link

stale bot commented Oct 21, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Oct 21, 2019
@sdetweil
Copy link
Collaborator

i have submitted my upgrade and install script enhancements as pr #1789

@stale stale bot removed the wontfix label Oct 24, 2019
@MichMich
Copy link
Collaborator

MichMich commented Oct 25, 2019

@sdetweil I've merged that PR. Can I close this one?

@sdetweil
Copy link
Collaborator

I am ok with closing, but I didn't open it.

@stale
Copy link

stale bot commented Dec 24, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Dec 24, 2019
@MichMich MichMich closed this Dec 28, 2019
@roramirez
Copy link
Contributor

I think we should keep the custom.css.sample in css

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants