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

Added support in CInitOptions to pass logging/filter modules to VSLog… #24107

Merged
merged 1 commit into from Sep 5, 2019

Conversation

@angelortiz1007
Copy link
Contributor

angelortiz1007 commented Aug 29, 2019

…ger.

Preliminary implementation of #23754. Added support in CInitOptions to take a **pmoduleList (array of string modules) and the size of the array string. Added logic to covert the **pmoduleList in HoloLens to a Vec.

Pending: implement how the passed in list of modules are used for filtering.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #23754__ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@angelortiz1007 angelortiz1007 force-pushed the angelortiz1007:servo-vslogger branch from 5c81c4b to 5c4d336 Aug 30, 2019
@angelortiz1007
Copy link
Contributor Author

angelortiz1007 commented Aug 30, 2019

implementation has been tested in the following way:

  1. In HoloLens, servo.cpp was modified as follows:
Servo::Servo(GLsizei width, GLsizei height, ServoDelegate &aDelegate)
    : mWindowHeight(height), mWindowWidth(width), mDelegate(aDelegate) {

  capi::CInitOptions o;
  o.args = "--pref dom.webxr.enabled";
  o.url = "https://servo.org";
  o.width = mWindowWidth;
  o.height = mWindowHeight;
  o.density = 1.0;
  o.enable_subpixel_text_antialiasing = false;
  o.vr_pointer = NULL;

  // 7 filter modules.
  static char *pfilters[64] = {
	  "servo",
	  "simpleservo", 
	  "simpleservo::jniapi",
	  "simpleservo::gl_glue::egl",
	  // Show JS errors by default.
	  "script::dom::bindings::error",
	  // Show GL errors by default.
	  "canvas::webgl_thread",
	  "compositing::compositor",
	  "constellation::constellation",
  };

  o.vslogger_mod_list = pfilters;	// servo log modules
  o.vslogger_mod_size = 7;			// Important: Number of modules in pfilters

mach build -d --uwp performed, HoloLens built and executed. Verified application did not crash.

Witnessed a Rust warning originating from simpleservo when reload/stop was pressed on HoloLens application.

Hololens was modified so that "simpleservo" was commented out. Hololens was built, executed and again, the "reload/stop" buttons pressed in the Hololens application. NO Rust wanrnings appeard for "simpleservo"

  1. The above code was modified so that the following were commented out:
  o.vslogger_mod_list = pfilters;	// servo log modules
  o.vslogger_mod_size = 7;			// Important: Number of modules in pfilters

Hololens was rebuilt and tested. Many more Rust warning messages appeared.

Based on the above 2 tests, this issue is ready for testing.

ports/libsimpleservo/capi/src/lib.rs Outdated Show resolved Hide resolved
ports/libsimpleservo/capi/src/lib.rs Outdated Show resolved Hide resolved
ports/libsimpleservo/capi/src/lib.rs Outdated Show resolved Hide resolved
ports/libsimpleservo/capi/src/lib.rs Outdated Show resolved Hide resolved
ports/libsimpleservo/capi/src/vslogger.rs Outdated Show resolved Hide resolved
ports/libsimpleservo/capi/src/vslogger.rs Outdated Show resolved Hide resolved
ports/libsimpleservo/capi/src/vslogger.rs Outdated Show resolved Hide resolved
ports/libsimpleservo/capi/src/vslogger.rs Outdated Show resolved Hide resolved
support/hololens/ServoApp/ServoControl/Servo.cpp Outdated Show resolved Hide resolved
support/hololens/ServoApp/ServoControl/Servo.cpp Outdated Show resolved Hide resolved
@angelortiz1007 angelortiz1007 force-pushed the angelortiz1007:servo-vslogger branch 2 times, most recently from ca87923 to cf6b569 Sep 3, 2019
@angelortiz1007
Copy link
Contributor Author

angelortiz1007 commented Sep 3, 2019

Sorry, accidently resolved implementing this feature on non-windows.

@angelortiz1007
Copy link
Contributor Author

angelortiz1007 commented Sep 4, 2019

I have a question on the non-windows logging. It seems that init_logger() is the key function. The following init_logger() function has several setup/functions to initiate the VSLogger defined in vslogger.rs.

#[cfg(target_os = "windows")]
fn init_logger(_modules: &[*const c_char]) {
    use crate::vslogger::LOG_MODULE_FILTERS;
    use log::LevelFilter;
    use std::sync::Once;
    use vslogger::VSLogger;

    static LOGGER: VSLogger = VSLogger;
    static LOGGER_INIT: Once = Once::new();

    if !_modules.is_empty() {
        *LOG_MODULE_FILTERS.lock().unwrap() = _modules
            .iter()
            .map(|_modules| unsafe { CStr::from_ptr(*_modules).to_string_lossy().into_owned() })
            .collect::<Vec<_>>();
    }

    LOGGER_INIT.call_once(|| {
        log::set_logger(&LOGGER)
            .map(|_| log::set_max_level(LevelFilter::Debug))
            .unwrap();
    });
}

However, the non-windows init_logger() does not seem to use any of the VSLogger code. See below.

#[cfg(not(target_os = "windows"))]
fn init_logger(_modules: &[*const c_char]) {
    crate::env_logger::init();
}

What are we looking to do within the non-windows init_logger() function once the signature has changed to accept a "_modules: &[*const c_char]" variable?

#[cfg(not(target_os = "windows"))]
fn init_logger(_modules: &[*const c_char]) {
    crate::env_logger::init();
}
@jdm
Copy link
Member

jdm commented Sep 4, 2019

We should accept the argument and ignore it for the time being. #23813 will allow us to make use of it.

@angelortiz1007 angelortiz1007 force-pushed the angelortiz1007:servo-vslogger branch from 74ba01a to 9401fa1 Sep 4, 2019
@angelortiz1007
Copy link
Contributor Author

angelortiz1007 commented Sep 4, 2019

OK. Just wanted to see if I was missing something.

@angelortiz1007
Copy link
Contributor Author

angelortiz1007 commented Sep 4, 2019

Completed modifying init_logger() for windows and non-windows builds.
Work should be completed.

@jdm
Copy link
Member

jdm commented Sep 4, 2019

A rebase is required before this can merge.

Copy link
Member

jdm left a comment

A few small code cleanups and then this should be mergeable!

ports/libsimpleservo/capi/src/lib.rs Outdated Show resolved Hide resolved
ports/libsimpleservo/capi/src/lib.rs Outdated Show resolved Hide resolved
ports/libsimpleservo/capi/src/lib.rs Outdated Show resolved Hide resolved
ports/libsimpleservo/capi/src/lib.rs Outdated Show resolved Hide resolved
ports/libsimpleservo/capi/src/vslogger.rs Outdated Show resolved Hide resolved
ports/libsimpleservo/capi/src/vslogger.rs Outdated Show resolved Hide resolved
ports/libsimpleservo/capi/src/vslogger.rs Outdated Show resolved Hide resolved
@angelortiz1007
Copy link
Contributor Author

angelortiz1007 commented Sep 4, 2019

Yes. A rebase has been done.
Let me know if all is OK.

@angelortiz1007 angelortiz1007 force-pushed the angelortiz1007:servo-vslogger branch from 41cb7dd to 067d582 Sep 4, 2019
@angelortiz1007
Copy link
Contributor Author

angelortiz1007 commented Sep 4, 2019

OK. Done with the changes and rebasing. :)

@jdm
Copy link
Member

jdm commented Sep 4, 2019

@angelortiz1007 Github still shows that this branch has a conflict with the master branch, so something didn't work as expected in the rebase process.

@angelortiz1007
Copy link
Contributor Author

angelortiz1007 commented Sep 4, 2019

OK. I wanted to make sure whatever step I took was correct. I believe I resolved the conflicts from the web interface.

@jdm
Copy link
Member

jdm commented Sep 4, 2019

It sounds like you didn't pull the latest changes from servo's master branch into your local clone's master branch before rebasing.

@angelortiz1007
Copy link
Contributor Author

angelortiz1007 commented Sep 4, 2019

OK, this is what I did:

  1. I brought my master repository with the current servo/sero by doing:
    git remote add upstream git@github.com:servo/servo.git
    git fetch upstream
    git rebase upstream/master

  2. After above completed, I:
    git checkout master
    git git pull origin master
    git rebase master servo-vslogger
    Here I got a merge issue that I believe I resolved.
    git rebase --continue
    git checkout servo-vslogger
    mach build -d --uwp
    Waiting to verify merge/build.

@angelortiz1007 angelortiz1007 force-pushed the angelortiz1007:servo-vslogger branch from 17f9a84 to 2873188 Sep 4, 2019
@angelortiz1007 angelortiz1007 force-pushed the angelortiz1007:servo-vslogger branch from 2873188 to 49934c0 Sep 5, 2019
@angelortiz1007
Copy link
Contributor Author

angelortiz1007 commented Sep 5, 2019

Completed the task. lazy_static removed from capi\Cargo.tom.

One issue in building HoloLens using mach build -d --uwp.
See below:
Contract.winmd
Midl:
All outputs are up-to-date.
All outputs are up-to-date.
All outputs are up-to-date.
C:\Program Files (x86)\Windows Kits\10\bin\10.0.17763.0\x64\midl.exe /metadata_dir "C:\Program Files (x86)\Win
dows Kits\10\References\10.0.17763.0\windows.foundation.foundationcontract\3.0.0.0" /winrt /W1 /nologo /char s
igned /env x64 /winmd "x64\Debug\Unmerged\XamlMetaDataProvider.winmd" /h "nul" /dlldata "nul" /iid "nul" /prox
y "nul" /notlb /client none /server none /enum_class /ns_prefix /target "NT60" /nomidl x64\Debug\XamlMetaData
Provider.idl
64 bit MIDLRT Processing C:\git-mozilla\servo-angelortiz1007\support\hololens\ServoApp\x64\Debug\XamlMetaDataP
rovider.idl
XamlMetaDataProvider.idl
2>C:\git-mozilla\servo-angelortiz1007\support\hololens\ServoApp\x64\Debug\XamlMetaDataProvider.idl(6): error MIDL2
011: [msg]unresolved type declaration [context]: Windows.UI.Xaml.Markup.IXamlMetadataProvider [ RuntimeClass 'Se
rvoApp.XamlMetaDataProvider' ] [C:\git-mozilla\servo-angelortiz1007\support\hololens\ServoApp\ServoApp.vcxproj]
2>Done Building Project "C:\git-mozilla\servo-angelortiz1007\support\hololens\ServoApp\ServoApp.vcxproj" (default
targets) -- FAILED.
1>Done Building Project "C:\git-mozilla\servo-angelortiz1007\support\hololens\ServoApp.sln" (default targets) -- F
AILED.

Build FAILED.

   "C:\git-mozilla\servo-angelortiz1007\support\hololens\ServoApp.sln" (default target) (1) ->
   "C:\git-mozilla\servo-angelortiz1007\support\hololens\ServoApp\ServoApp.vcxproj" (default target) (2) ->
   (Midl target) ->
     C:\git-mozilla\servo-angelortiz1007\support\hololens\ServoApp\x64\Debug\XamlMetaDataProvider.idl(6): error MID
   L2011: [msg]unresolved type declaration [context]: Windows.UI.Xaml.Markup.IXamlMetadataProvider [ RuntimeClass '
   ServoApp.XamlMetaDataProvider'  ] [C:\git-mozilla\servo-angelortiz1007\support\hololens\ServoApp\ServoApp.vcxpro
   j]

0 Warning(s)
1 Error(s)

Time Elapsed 00:00:01.39
Error running mach:

['build', '-d', '--uwp']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

CalledProcessError: Command 'C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\MSBuild\15.0\Bin\msbuild.exe /m /p:project=ServoApp .\support\hololens\ServoApp.sln /p:SolutionDir=.\support\hololens /p:Configuration=Debug /p:Platform=x64 /p:AppxBundle=Always;AppxBundlePlatforms=x64' returned non-zero exit status 1

File "C:\git-mozilla\servo-angelortiz1007\python\servo\build_commands.py", line 709, in build
build_uwp_hololens(target_triple, dev, msbuildinstalldir)
File "C:\git-mozilla\servo-angelortiz1007\python\servo\build_commands.py", line 963, in build_uwp_hololens
"/p:AppxBundle=Always;AppxBundlePlatforms=" + vs_platform])
File "C:\git-mozilla\servo-angelortiz1007\python\servo\command_base.py", line 195, in check_call
raise subprocess.CalledProcessError(status, ' '.join(*args))

@jdm
Copy link
Member

jdm commented Sep 5, 2019

My experience with that error is that rebuilding the project in VS makes it go away. In any case, it should be unrelated to the changes in this PR.

@jdm
Copy link
Member

jdm commented Sep 5, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

📌 Commit 49934c0 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

Testing commit 49934c0 with merge 7297478...

bors-servo added a commit that referenced this pull request Sep 5, 2019
Added support in CInitOptions to pass logging/filter modules to VSLog…

…ger.

<!-- Please describe your changes on the following line: -->
Preliminary implementation of #23754.  Added support in CInitOptions to take a **pmoduleList (array of string modules) and the size of the array string.  Added logic to covert the **pmoduleList in HoloLens to a Vec<String>.

Pending: implement how the passed in list of modules are used for filtering.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #23754__ (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24107)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

💔 Test failed - status-taskcluster

@CYBAI
Copy link
Collaborator

CYBAI commented Sep 5, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

💣 Failed to start rebuilding: Unknown error

@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

Testing commit 49934c0 with merge e81af16...

bors-servo added a commit that referenced this pull request Sep 5, 2019
Added support in CInitOptions to pass logging/filter modules to VSLog…

…ger.

<!-- Please describe your changes on the following line: -->
Preliminary implementation of #23754.  Added support in CInitOptions to take a **pmoduleList (array of string modules) and the size of the array string.  Added logic to covert the **pmoduleList in HoloLens to a Vec<String>.

Pending: implement how the passed in list of modules are used for filtering.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #23754__ (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24107)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing e81af16 to master...

@bors-servo bors-servo merged commit 49934c0 into servo:master Sep 5, 2019
3 checks passed
3 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
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.