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

add shared logs base directory #5409

Merged
merged 1 commit into from Aug 11, 2016
Merged

add shared logs base directory #5409

merged 1 commit into from Aug 11, 2016

Conversation

Ashton-W
Copy link
Contributor

@Ashton-W Ashton-W commented Jul 16, 2016

Adds FL_BUILDLOG_PATH environment variable to override the default base log path of ~/Library/Logs.

Half Implements #4989


Another PR will include changes to:

  • gym
  • scan
  • snapshot
  • fastlane/actions/xcodebuild

master...Ashton-W:aw-common-buildlog_path

@@ -66,6 +66,11 @@ def self.iterm?
!!ENV["ITERM_SESSION_ID"]
end

# Logs base directory
def self.buildlog_path
return ENV["FL_BUILDLOG_PATH"] || "~/Library/Logs"
Copy link
Member

Choose a reason for hiding this comment

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

Please use File.expand() here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean for both values or just the default with the ~? I left then unexpanded because they were in the places I call this from, the option defaults are unexpanded paths. For example gym prints a table of the options it's using and that table reads "~/Library/Logs/gym" if I expand it now it will be the complete path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I wasn't clear, I'd like your feedback on this again @KrauseFx

Copy link
Member

Choose a reason for hiding this comment

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

File.expand_path(ENV["FL_BUILDLOG_PATH"] || "~/Library/Logs") 👍

@KrauseFx
Copy link
Member

This looks great. However you'll need to split this up to 2 PRs, one for fastlane_core, and one for the tools, which will fail until we pushed a new fastlane_core release

@Ashton-W
Copy link
Contributor Author

OK. I've split up the commit and force pushed this PR to be part 1

@KrauseFx
Copy link
Member

Perfect, thanks 👍

@Ashton-W
Copy link
Contributor Author

Keep it mind, with the path now expanded all tools will go from this:

+----------------------+------------------------+
|             Summary for gym 1.6.4             |
+----------------------+------------------------+
| project              | ./test.xcodeproj       |
| scheme               | test                   |
| destination          | generic/platform=macOS |
| output_name          | test                   |
| clean                | false                  |
| output_directory     | .                      |
| silent               | false                  |
| use_legacy_build_api | false                  |
| buildlog_path        | ~/Library/Logs/gym     |
+----------------------+------------------------+

to something like this

+----------------------+-----------------------------------+
|             Summary for gym 1.6.4                        |
+----------------------+-----------------------------------+
| project              | ./test.xcodeproj                  |
| scheme               | test                              |
| destination          | generic/platform=macOS            |
| output_name          | test                              |
| clean                | false                             |
| output_directory     | .                                 |
| silent               | false                             |
| use_legacy_build_api | false                             |
| buildlog_path        | /Users/Felix/Library/Logs/gym     |
+----------------------+-----------------------------------+

@KrauseFx
Copy link
Member

Ugh, that's not ideal, good point.

@Ashton-W
Copy link
Contributor Author

Like the others helpers, we could instead put the onus on callers to expand the path as needed.

@Ashton-W
Copy link
Contributor Author

@KrauseFx I took out the commit that expanded the path. Let's keep it in mind for Part 2.

@Ashton-W
Copy link
Contributor Author

Would be great to get this in soon so I move on to part 2 🙋

@KrauseFx
Copy link
Member

Sorry for the delay @Ashton-W, I'm reviewing your PR now 👍

@KrauseFx KrauseFx merged commit a6f1b31 into fastlane:master Aug 11, 2016
@KrauseFx
Copy link
Member

This is perfect, really sorry for the delay again, and I can't wait for step 2 👍

KrauseFx added a commit that referenced this pull request Aug 11, 2016
* Implement common Logs directory in FastlaneCore (#5409)
* Update commander dependency
KrauseFx added a commit that referenced this pull request Aug 11, 2016
* Implement common Logs directory in FastlaneCore (#5409)
* Update commander dependency
@KrauseFx
Copy link
Member

Thanks, this is already in the latest release. This should be documented as well 👍

@fastlane fastlane locked and limited conversation to collaborators Feb 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants