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.

Sign up for GitHub

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

Jump to bottom

Remove libs from search path #3593

Closed
wants to merge 1 commit into from
Closed

Remove libs from search path #3593

wants to merge 1 commit into from

Conversation

splattael
Copy link
Contributor

@splattael splattael commented Nov 26, 2016 •
edited

This PR removes libs directory from the search path.

#2788 should have removed libs, I guess, it only added lib.

/cc @ysbaddaden

@asterite
Copy link
Member

My idea was that for at least one versions both lib and libs are in the path, so upgrading to Crystal 0.20.x won't break existing projects. We can merge this at 0.21.0

@splattael
Copy link
Contributor Author

splattael commented Nov 27, 2016 •
edited

Ok, but I think we have to swap libs and lib in the search path then. Otherwise upgrading breaks. It did break for me at least on several repos.

@splattael splattael mentioned this pull request Nov 27, 2016
Merged
@splattael
Copy link
Contributor Author

@asterite Thanks for merging #3598 💜

I've rebased this PR so it can merged before 0.21.0 ;)

@splattael
Copy link
Contributor Author

One build fails:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

The other 3 builds are just fine.

luislavena
luislavena reviewed Nov 28, 2016
View reviewed changes
Dockerfile
@@ -10,7 +10,7 @@ ADD . /opt/crystal-head

WORKDIR /opt/crystal-head
ENV CRYSTAL_CONFIG_VERSION HEAD
ENV CRYSTAL_CONFIG_PATH lib:libs:/opt/crystal-head/src:/opt/crystal-head/libs
ENV CRYSTAL_CONFIG_PATH lib:/opt/crystal-head/src:/opt/crystal-head/libs
Copy link
Contributor

Choose a reason for hiding this comment

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

@asterite question, what is inside /opt/crystal-head/libs? Perhaps that can be removed too?

Copy link
Contributor Author

@splattael splattael Nov 28, 2016 •
edited

Choose a reason for hiding this comment

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

@luislavena CRYSTAL_CONFIG_PATH is only used for Crystal::Config.path.
I've removed the complete line CRYSTAL_CONFIG_PATH and docker build . still produces a valid crystal compiler.

I think we can remove the whole line.

@splattael
Copy link
Contributor Author

splattael commented Dec 25, 2016 •
edited

Partially applied in 663c75d.

I make another PR to remove libs from .gitignore during crystal init.

Edit: Done in #3777

@splattael splattael closed this Dec 25, 2016
@splattael splattael mentioned this pull request Dec 25, 2016
Merged
ysbaddaden pushed a commit that referenced this pull request Dec 25, 2016
@splattael @ysbaddaden
Do not add libs/ to .gitignore (#3777)
ee52cce 
As `libs/` was removed in 663c75d we do not need to gitignore `libs/`
anymore.

See also #3593
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luislavena luislavena luislavena left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@splattael @asterite @luislavena