-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Built sandcastle using unminified cesium #5842
Changes from 2 commits
b4069fb
5feb21d
830af5b
3bb75d3
17151ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,10 +8,12 @@ | |
<script type="text/javascript" src="../Sandcastle-header.js"></script> | ||
<script type="text/javascript" src="../../../ThirdParty/requirejs-2.1.20/require.js"></script> | ||
<script type="text/javascript"> | ||
require.config({ | ||
baseUrl : '../../../Source', | ||
waitSeconds : 60 | ||
}); | ||
if(typeof require === "function") { | ||
require.config({ | ||
baseUrl : '../../../Source', | ||
waitSeconds : 60 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we bump this to 120 or something? I know this PR is about making this faster not slower, but sometimes (especially loading legacy releases off the site) this timeout is too short. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can make this a little longer, but this change will only affect future release builds. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, the the require call shouldn't happen in the built version regardless, so it will only slow down the dev version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this was kind of a tangential comment, I know you didn't set this value in this PR. |
||
}); | ||
} | ||
</script> | ||
</head> | ||
<body class="sandcastle-loading" data-sandcastle-bucket="bucket-requirejs.html"> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -113,7 +113,10 @@ gulp.task('build-watch', function() { | |
}); | ||
|
||
gulp.task('buildApps', function() { | ||
return buildCesiumViewer(); | ||
return buildCesiumViewer() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason we can't build these in parallel via a |
||
.then(function () { | ||
return buildSandcastle(); | ||
}); | ||
}); | ||
|
||
gulp.task('clean', function(done) { | ||
|
@@ -1153,6 +1156,22 @@ var sandcastleJsHintOptions = ' + JSON.stringify(primary, null, 4) + ';'; | |
fs.writeFileSync(path.join('Apps', 'Sandcastle', 'jsHintOptions.js'), contents); | ||
} | ||
|
||
function buildSandcastle() { | ||
return gulp.src([ | ||
'Apps/Sandcastle/**' | ||
]) | ||
// Replace require Source with pre-built Cesium | ||
.pipe(gulpReplace('../../../ThirdParty/requirejs-2.1.20/require.js', '../../../CesiumUnminified/Cesium.js')) | ||
// Use unminified cesium instead of source | ||
.pipe(gulpReplace('Source/Cesium', 'CesiumUnminified')) | ||
// Fix relative paths for new location | ||
.pipe(gulpReplace('../../Source', '../../../Source')) | ||
.pipe(gulpReplace('../../ThirdParty', '../../../ThirdParty')) | ||
.pipe(gulpReplace('../../SampleData', '../../../../Apps/SampleData')) | ||
.pipe(gulpReplace('Build/Documentation', 'Documentation')) | ||
.pipe(gulp.dest('Build/Apps/Sandcastle')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think all of the JPGs and PNGs are getting mangled here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, the root folder's |
||
} | ||
|
||
function buildCesiumViewer() { | ||
var cesiumViewerOutputDirectory = 'Build/Apps/CesiumViewer'; | ||
var cesiumViewerStartup = path.join(cesiumViewerOutputDirectory, 'CesiumViewerStartup.js'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful here. On line
6
above,baseUrl
is set to be theSource
folder, and your build script already moves this one level higher in the transformed version of this file. So, line21
here has one too many parent-folder references, I believe. (On my machine I serve Cesium from a URL path, so I get broken when there are too many..
instances).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change will fix it for me, at least this is the only one I see that looks like it goes up too many levels. I didn't try making the change myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The location reflects what it should be when the app is copied to the
Build/Apps
folder. There is no reason to use this location in the unbuilt version, so I used the built path so it didn't need to be modified in the build process, but that not very clear.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But take a look at what actually gets built. The built version of this file, on line 6, you can see
baseUrl
has been transformed by the build from../../Source
to../../../Source
. So, this URL is then${built Sandcastle folder}/../../../Source/../../Build/CesiumUnminified
. In this case, it correctly finds theSource
folder, but then it goes "too high" aboveSource
when looking forBuild
.This works on your machine because you bump into the root path there and it doesn't matter how many extra
..
s there are. But on my machine, I install Cesium in a sub-folder/Cesium
, partly because it is the only way to catch mistakes like these.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see!