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

Remove the need of the dos2unix program #164

Closed
mikygee opened this issue Jul 29, 2020 · 10 comments
Closed

Remove the need of the dos2unix program #164

mikygee opened this issue Jul 29, 2020 · 10 comments

Comments

@mikygee
Copy link
Contributor

mikygee commented Jul 29, 2020

Spine: 1.2.13

Hello,
It would be nice if you removed the need to use dos2unix

I don't want to install this program on my system and on the other hand there is no need for it when I download the tar.gz

Options possible:

  • Remove the dos2unix
  • Make a switch -nodos2unix or something like that which would disable the need for this program
  • Add some code that would convert all lines ending by \r\n with \n and remove the need for dos2unix

The latest option seems good.

Thank you

@netniV
Copy link
Member

netniV commented Jul 31, 2020

Does spine use this itself? I will have to check. It may be used by third party scripts.

@mikygee
Copy link
Contributor Author

mikygee commented Jul 31, 2020

Hello, It's in the INSTALL file

6. Once on the package selection section make sure to select the following (TIP: use the search!):
      * dos2unix

And the bootstrap

# Check for dos2unix
which dos2unix > /dev/null 2>&1
[ $? -gt 0 ] && echo "FATAL: Unable to locate dos2unix utility" && exit -1

I'm downloading the cacti-spine-1.2.13.tar.gz from the site to a unix platform so the file is correctly formatted.

I think bootstrap should test if it's a windows platform, if yes make the dos2unix program a prerequisite, else skip the dos2unix presence verification.

@mikygee
Copy link
Contributor Author

mikygee commented Aug 11, 2020

In bootstrap I modified the code like this
unamestr=$(uname)

if [ $unamestr != 'OpenBSD' ]; then
  # Check for dos2unix
  which dos2unix > /dev/null 2>&1
  [ $? -gt 0 ] && echo "FATAL: Unable to locate dos2unix utility" && exit -1
  find . -type f -exec dos2unix --d2u \{\} \; > /dev/null 2>&1
fi

Honestly, this dos2unix command is just usefull when you're on a Windows platform so the condition should be something like
if [ $unamestr == 'Windows' ]; then

If you're on mac or linux you can assume that \r\n won't be added during the download.

@netniV
Copy link
Member

netniV commented Aug 11, 2020

That does seem reasonable. I guess the precaution is there in case we actually package on a windows system. It may even really be a hang over from the SVN days because SVN used to convert line feeds. I don't think that Windows should actually convert the files imho, so we might be better removing it altogether, what do you think @TheWitness, @cigamit or @browniebraun ?

@cigamit
Copy link
Member

cigamit commented Aug 11, 2020

While I don't think "I don't want to install this program on my system" is a valid reason for removing it, I don't believe it has been required on the Linux side since we moved to git. I tested it on Centos last night and didn't have any issues. I would wait until we get feedback from the Windows side though.

@netniV
Copy link
Member

netniV commented Aug 11, 2020

I don't think "I don't want to install this program on my system" is a valid reason for removing it

That was my first thought and I would agree with that sentiment, though if it's unneeded then removing unnecessary prerequisites is always a good thing in my book 👍

@TheWitness
Copy link
Member

dos2unix is everywhere. I use it all the time. The need for this was due to a legacy, and still relevant today of people who are still performing development and merge activities on Windows and those who prefer just to use the GitHub Desktop.

Unfortunately, decades ago, Microsoft decided to make the backslash character a directory separator, and thus began a multi-decade long path of incompatibility that continues today. To add insult to injury, they also had to adopt the Carriage Return (yea, for old guys like me, that was the arm on the manual typewriter that you had to pull to go from the end of a line to the next one). Oh well, I thought I would write this down just in case the new kiddies were wondering what the hell happened in the late 70's and early 80's.

@netniV
Copy link
Member

netniV commented Aug 14, 2020

Drugs

@TheWitness
Copy link
Member

The more I use windows, and I use it every day, the more I dislike it. But could not work without unless, I do.

@mikygee
Copy link
Contributor Author

mikygee commented Aug 26, 2020

Hello TheWitness,
I think the idea would be to have a switch for Windows

unamestr=$(uname)
if [ $unamestr == 'Windows' ]; then
  # Check for dos2unix
  which dos2unix > /dev/null 2>&1
  [ $? -gt 0 ] && echo "FATAL: Unable to locate dos2unix utility" && exit -1
  find . -type f -exec dos2unix --d2u \{\} \; > /dev/null 2>&1
fi

I'm not sure about what gives unamestr=$(uname) on Windows, can you test it ?
Would you consider implement this code ?

When users are on Linux/Unix only there is no reason why a \r\n would appear, except if they download the cacti on their Windows desktop, unzip it, open a file and save it with notepad and then reupload it on a linux machine.

TheWitness added a commit that referenced this issue Nov 20, 2020
Remove the need of the dos2unix program
@TheWitness TheWitness added this to the v1.2.16 milestone Nov 20, 2020
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

4 participants