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

Update heartbeats and energymon profiler dependencies #9926

Merged
merged 1 commit into from Mar 10, 2016

Conversation

@connorimes
Copy link
Contributor

connorimes commented Mar 8, 2016

Heartbeats now on crates.io.
Updates to energymon interface for energy profiling.
Profiling script for Android.

Review on Reviewable

@highfive
Copy link

highfive commented Mar 8, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@asajeffrey
Copy link
Member

asajeffrey commented Mar 8, 2016

@larsbergstrom can you take a look at tests/heartbeats/characterize_android.py?

@asajeffrey
Copy link
Member

asajeffrey commented Mar 8, 2016

Reviewed 6 of 9 files at r1.
Review status: 6 of 9 files reviewed at latest revision, 2 unresolved discussions.


components/script/parser.out, line 0 [r1] (raw file):
Do you know what caused this diff?


tests/heartbeats/characterize.py, line 26 [r1] (raw file):
A test commented out?


Comments from the review on Reviewable.io

@asajeffrey
Copy link
Member

asajeffrey commented Mar 8, 2016

Mostly looks fine. It'd be nice to get someone who knows their way round the android build to look at it. Do you know why parser.out changed?

@connorimes
Copy link
Contributor Author

connorimes commented Mar 8, 2016

I don't have any idea what that parser thing is. I commented out that profiler category because it doesn't use the profiling interface properly and therefore doesn't behave like the others. It screws up data in log files and the chart generation by the process_logs python script as a result.

Basically all the heartbeats changes are just better build support, esp. for cross compiling (also now using Alex's cmake crate to help). Also, I published both the bindings and abstraction crates on crates.io. Additionally, they're relicensed to dual MIT/Apache-2.0.

The real changes are in the energymon-related crates and associated native C code. They are also re-licensed now. Energy monitoring capabilities are only enabled with the energy-profiling feature, which is disabled by default.

@connorimes
Copy link
Contributor Author

connorimes commented Mar 8, 2016

Review status: 6 of 9 files reviewed at latest revision, 2 unresolved discussions.


tests/heartbeats/characterize.py, line 26 [r1] (raw file):
This is intentional - this profiler category doesn't use the profiling interface properly and causes problems.


Comments from the review on Reviewable.io

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Mar 8, 2016

Reviewed 1 of 9 files at r1.
Review status: 7 of 9 files reviewed at latest revision, 2 unresolved discussions.


components/script/parser.out, line [r1] (raw file):
This file was removed in #9817. Does this PR need a rebase?


Comments from the review on Reviewable.io

@connorimes connorimes force-pushed the connorimes:update-em-interface-hbs-crates branch from 2bf1e66 to 5215573 Mar 8, 2016
@asajeffrey
Copy link
Member

asajeffrey commented Mar 8, 2016

I'm going to regret asking this, but... over in https://github.com/libheartbeats/heartbeats-simple/blob/master/LICENSE the copyright holder is given as "The University of Chicago". Is this correct?

@connorimes
Copy link
Contributor Author

connorimes commented Mar 8, 2016

Ah, I didn't even realize I had added a deleted file back in. Maybe something happened pushing/popping my git stash when I pulled to get the latest changes before making my branch. Sorry about that - good catch. I've deleted the file and rebased my commit.

@asajeffrey
Copy link
Member

asajeffrey commented Mar 8, 2016

Reviewed 1 of 9 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@connorimes
Copy link
Contributor Author

connorimes commented Mar 8, 2016

We chose the 3-clause BSD license when we released Heartbeats source as part of other work. The heartbeats-simple project is originally based on that source. The BSD license allows source redistribution as long as the LICENSE file is retained. We've previously included these projects in servo, so this is not new.
When I began development on the energymon projects, I just applied the same license, but I can easily relicense energymon since I'm the only contributor ever.
All the rust code (sys bindings and abstraction crates) are dual-licensed under MIT/Apache-2.0 which I started work on last summer while I was with Mozilla. They were originally MIT only, which is what was recommended to me at the time.

@asajeffrey
Copy link
Member

asajeffrey commented Mar 8, 2016

Is UoC the copyright holder? I'm a bit concerned that if they are, we need someone to officially bless the licensing.

@connorimes
Copy link
Contributor Author

connorimes commented Mar 8, 2016

I'm a graduate student at UofC and primary developer of the libraries. Per the licensing, nobody at UofC's end has to bless the redistribution, nor is it a copyleft license. If you need blessing from inside Mozilla, I think @larsbergstrom can provide that.

@asajeffrey
Copy link
Member

asajeffrey commented Mar 8, 2016

Sorry to harp on about this, but is UoC the copyright holder? If they are, we need to think about the licensing. If not, could you update the LICENSE files?

@connorimes connorimes force-pushed the connorimes:update-em-interface-hbs-crates branch from 5215573 to 2ab12a3 Mar 8, 2016
@connorimes
Copy link
Contributor Author

connorimes commented Mar 8, 2016

I've updated the license files for the energymon project to be dual MIT/Apache-2.0.

@asajeffrey
Copy link
Member

asajeffrey commented Mar 8, 2016

Thanks!

@asajeffrey
Copy link
Member

asajeffrey commented Mar 8, 2016

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@asajeffrey
Copy link
Member

asajeffrey commented Mar 8, 2016

@larsbergstrom did tests/heartbeats/characterize_android.py look good to you?

@bors-servo try

@bors-servo
Copy link
Contributor

bors-servo commented Mar 8, 2016

Trying commit 2ab12a3 with merge 2af98a4...

bors-servo added a commit that referenced this pull request Mar 8, 2016
Update heartbeats and energymon profiler dependencies

Heartbeats now on crates.io.
Updates to energymon interface for energy profiling.
Profiling script for Android.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9926)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 8, 2016

Updates to energymon interface for energy profiling.
Profiling script for Android.
@connorimes connorimes force-pushed the connorimes:update-em-interface-hbs-crates branch from 3cad3cd to f31e884 Mar 9, 2016
@connorimes
Copy link
Contributor Author

connorimes commented Mar 10, 2016

I rebased this yesterday afternoon. Did it become unmergeable again? Or is something else missing?

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Mar 10, 2016

@bors-servo r=asajeffrey

Unlike nearly every other action on GitHub, rebases / pushing extra commits to a branch does not send any update mails.

@bors-servo
Copy link
Contributor

bors-servo commented Mar 10, 2016

📌 Commit f31e884 has been approved by asajeffrey

@connorimes
Copy link
Contributor Author

connorimes commented Mar 10, 2016

I knew that but I thought highfive automatically picked it up. Thanks.

@bors-servo
Copy link
Contributor

bors-servo commented Mar 10, 2016

Testing commit f31e884 with merge 18f0a9a...

bors-servo added a commit that referenced this pull request Mar 10, 2016
…ajeffrey

Update heartbeats and energymon profiler dependencies

Heartbeats now on crates.io.
Updates to energymon interface for energy profiling.
Profiling script for Android.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9926)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 10, 2016

💔 Test failed - status-appveyor

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Mar 10, 2016

@bors-servo retry

  • AppVeyor misconfiguration
@bors-servo
Copy link
Contributor

bors-servo commented Mar 10, 2016

Testing commit f31e884 with merge 396812b...

bors-servo added a commit that referenced this pull request Mar 10, 2016
…ajeffrey

Update heartbeats and energymon profiler dependencies

Heartbeats now on crates.io.
Updates to energymon interface for energy profiling.
Profiling script for Android.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9926)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 10, 2016

💔 Test failed - linux-rel

@asajeffrey
Copy link
Member

asajeffrey commented Mar 10, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Mar 10, 2016

Previous build results for android, gonk, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor are reusable. Rebuilding only linux-rel...

@bors-servo
Copy link
Contributor

bors-servo commented Mar 10, 2016

@bors-servo bors-servo merged commit f31e884 into servo:master Mar 10, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@asajeffrey
Copy link
Member

asajeffrey commented Mar 10, 2016

Yay! Thanks, @connorimes!

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.